Re: [PULL 19/45] memory: clamp cached translation in case it points to an MMIO region

[Michael S. Tsirkin]

On Tue, Dec 15, 2020 at 12:54:19PM -0500, Paolo Bonzini wrote:
> In using the address_space_translate_internal API, address_space_cache_init
> forgot one piece of advice that can be found in the code for
> address_space_translate_internal:
> 
>     /* MMIO registers can be expected to perform full-width accesses based 
> only
>      * on their address, without considering adjacent registers that could
>      * decode to completely different MemoryRegions.  When such registers
>      * exist (e.g. I/O ports 0xcf8 and 0xcf9 on most PC chipsets), MMIO
>      * regions overlap wildly.  For this reason we cannot clamp the accesses
>      * here.
>      *
>      * If the length is small (as is the case for address_space_ldl/stl),
>      * everything works fine.  If the incoming length is large, however,
>      * the caller really has to do the clamping through memory_access_size.
>      */
> 
> address_space_cache_init is exactly one such case where "the incoming length
> is large", therefore we need to clamp the resulting length---not to
> memory_access_size though, since we are not doing an access yet, but to
> the size of the resulting section.  This ensures that subsequent accesses
> to the cached MemoryRegionSection will be in range.
> 
> With this patch, the enclosed testcase notices that the used ring does
> not fit into the MSI-X table and prints a "qemu-system-x86_64: Cannot map 
> used"
> error.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Cc: qemu-stable@nongnu.org

> ---
>  softmmu/physmem.c       | 10 ++++++++
>  tests/qtest/fuzz-test.c | 51 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 3027747c03..2cd1de4a2c 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -3255,6 +3255,7 @@ int64_t address_space_cache_init(MemoryRegionCache 
> *cache,
>      AddressSpaceDispatch *d;
>      hwaddr l;
>      MemoryRegion *mr;
> +    Int128 diff;
>  
>      assert(len > 0);
>  
> @@ -3263,6 +3264,15 @@ int64_t address_space_cache_init(MemoryRegionCache 
> *cache,
>      d = flatview_to_dispatch(cache->fv);
>      cache->mrs = *address_space_translate_internal(d, addr, &cache->xlat, 
> &l, true);
>  
> +    /*
> +     * cache->xlat is now relative to cache->mrs.mr, not to the section 
> itself.
> +     * Take that into account to compute how many bytes are there between
> +     * cache->xlat and the end of the section.
> +     */
> +    diff = int128_sub(cache->mrs.size,
> +                   int128_make64(cache->xlat - 
> cache->mrs.offset_within_region));
> +    l = int128_get64(int128_min(diff, int128_make64(l)));
> +
>      mr = cache->mrs.mr;
>      memory_region_ref(mr);
>      if (memory_access_is_direct(mr, is_write)) {
> diff --git a/tests/qtest/fuzz-test.c b/tests/qtest/fuzz-test.c
> index 87b72307a5..cdb1100a0b 100644
> --- a/tests/qtest/fuzz-test.c
> +++ b/tests/qtest/fuzz-test.c
> @@ -48,6 +48,55 @@ static void 
> test_lp1878642_pci_bus_get_irq_level_assert(void)
>      qtest_quit(s);
>  }
>  
> +/*
> + * Here a MemoryRegionCache pointed to an MMIO region but had a
> + * larger size than the underlying region.
> + */
> +static void test_mmio_oob_from_memory_region_cache(void)
> +{
> +    QTestState *s;
> +
> +    s = qtest_init("-M pc-q35-5.2 -display none -m 512M "
> +                "-device virtio-scsi,num_queues=8,addr=03.0 ");
> +
> +    qtest_outl(s, 0xcf8, 0x80001811);
> +    qtest_outb(s, 0xcfc, 0x6e);
> +    qtest_outl(s, 0xcf8, 0x80001824);
> +    qtest_outl(s, 0xcf8, 0x80001813);
> +    qtest_outl(s, 0xcfc, 0xa080000);
> +    qtest_outl(s, 0xcf8, 0x80001802);
> +    qtest_outl(s, 0xcfc, 0x5a175a63);
> +    qtest_outb(s, 0x6e08, 0x9e);
> +    qtest_writeb(s, 0x9f003, 0xff);
> +    qtest_writeb(s, 0x9f004, 0x01);
> +    qtest_writeb(s, 0x9e012, 0x0e);
> +    qtest_writeb(s, 0x9e01b, 0x0e);
> +    qtest_writeb(s, 0x9f006, 0x01);
> +    qtest_writeb(s, 0x9f008, 0x01);
> +    qtest_writeb(s, 0x9f00a, 0x01);
> +    qtest_writeb(s, 0x9f00c, 0x01);
> +    qtest_writeb(s, 0x9f00e, 0x01);
> +    qtest_writeb(s, 0x9f010, 0x01);
> +    qtest_writeb(s, 0x9f012, 0x01);
> +    qtest_writeb(s, 0x9f014, 0x01);
> +    qtest_writeb(s, 0x9f016, 0x01);
> +    qtest_writeb(s, 0x9f018, 0x01);
> +    qtest_writeb(s, 0x9f01a, 0x01);
> +    qtest_writeb(s, 0x9f01c, 0x01);
> +    qtest_writeb(s, 0x9f01e, 0x01);
> +    qtest_writeb(s, 0x9f020, 0x01);
> +    qtest_writeb(s, 0x9f022, 0x01);
> +    qtest_writeb(s, 0x9f024, 0x01);
> +    qtest_writeb(s, 0x9f026, 0x01);
> +    qtest_writeb(s, 0x9f028, 0x01);
> +    qtest_writeb(s, 0x9f02a, 0x01);
> +    qtest_writeb(s, 0x9f02c, 0x01);
> +    qtest_writeb(s, 0x9f02e, 0x01);
> +    qtest_writeb(s, 0x9f030, 0x01);
> +    qtest_outb(s, 0x6e10, 0x00);
> +    qtest_quit(s);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *arch = qtest_get_arch();
> @@ -59,6 +108,8 @@ int main(int argc, char **argv)
>                         test_lp1878263_megasas_zero_iov_cnt);
>          qtest_add_func("fuzz/test_lp1878642_pci_bus_get_irq_level_assert",
>                         test_lp1878642_pci_bus_get_irq_level_assert);
> +        qtest_add_func("fuzz/test_mmio_oob_from_memory_region_cache",
> +                       test_mmio_oob_from_memory_region_cache);
>      }
>  
>      return g_test_run();
> -- 
> 2.26.2
> 
> 
>