Re: [PATCH v4 1/2] vfio: move the function vfio_get_xlat_addr() to memory.c

[Alex Williamson]

On Fri, 28 Oct 2022 09:50:10 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On Fri, Oct 28, 2022 at 5:11 AM Alex Williamson
> <alex.williamson@redhat.com> wrote:
> >
> > On Thu, 27 Oct 2022 15:40:31 +0800
> > Cindy Lu <lulu@redhat.com> wrote:
> >  
> > > Move the function vfio_get_xlat_addr to softmmu/memory.c, and
> > > change the name to memory_get_xlat_addr().So we can use this
> > > function in other devices,such as vDPA device.
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  hw/vfio/common.c      | 92 ++-----------------------------------------
> > >  include/exec/memory.h |  4 ++
> > >  softmmu/memory.c      | 84 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 92 insertions(+), 88 deletions(-)
> > >
> > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > index ace9562a9b..2b5a9f3d8d 100644
> > > --- a/hw/vfio/common.c
> > > +++ b/hw/vfio/common.c
> > > @@ -574,92 +574,6 @@ static bool 
> > > vfio_listener_skipped_section(MemoryRegionSection *section)
> > >             section->offset_within_address_space & (1ULL << 63);
> > >  }
> > >
> > > -/* Called with rcu_read_lock held.  */
> > > -static bool vfio_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > -                               ram_addr_t *ram_addr, bool *read_only)
> > > -{
> > > -    MemoryRegion *mr;
> > > -    hwaddr xlat;
> > > -    hwaddr len = iotlb->addr_mask + 1;
> > > -    bool writable = iotlb->perm & IOMMU_WO;
> > > -
> > > -    /*
> > > -     * The IOMMU TLB entry we have just covers translation through
> > > -     * this IOMMU to its immediate target.  We need to translate
> > > -     * it the rest of the way through to memory.
> > > -     */
> > > -    mr = address_space_translate(&address_space_memory,
> > > -                                 iotlb->translated_addr,
> > > -                                 &xlat, &len, writable,
> > > -                                 MEMTXATTRS_UNSPECIFIED);
> > > -    if (!memory_region_is_ram(mr)) {
> > > -        error_report("iommu map to non memory area %"HWADDR_PRIx"",
> > > -                     xlat);
> > > -        return false;
> > > -    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > -        RamDiscardManager *rdm = 
> > > memory_region_get_ram_discard_manager(mr);
> > > -        MemoryRegionSection tmp = {
> > > -            .mr = mr,
> > > -            .offset_within_region = xlat,
> > > -            .size = int128_make64(len),
> > > -        };
> > > -
> > > -        /*
> > > -         * Malicious VMs can map memory into the IOMMU, which is expected
> > > -         * to remain discarded. vfio will pin all pages, populating 
> > > memory.
> > > -         * Disallow that. vmstate priorities make sure any 
> > > RamDiscardManager
> > > -         * were already restored before IOMMUs are restored.
> > > -         */
> > > -        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > -            error_report("iommu map to discarded memory (e.g., unplugged 
> > > via"
> > > -                         " virtio-mem): %"HWADDR_PRIx"",
> > > -                         iotlb->translated_addr);
> > > -            return false;
> > > -        }
> > > -
> > > -        /*
> > > -         * Malicious VMs might trigger discarding of IOMMU-mapped 
> > > memory. The
> > > -         * pages will remain pinned inside vfio until unmapped, 
> > > resulting in a
> > > -         * higher memory consumption than expected. If memory would get
> > > -         * populated again later, there would be an inconsistency 
> > > between pages
> > > -         * pinned by vfio and pages seen by QEMU. This is the case until
> > > -         * unmapped from the IOMMU (e.g., during device reset).
> > > -         *
> > > -         * With malicious guests, we really only care about pinning more 
> > > memory
> > > -         * than expected. RLIMIT_MEMLOCK set for the user/process can 
> > > never be
> > > -         * exceeded and can be used to mitigate this problem.
> > > -         */
> > > -        warn_report_once("Using vfio with vIOMMUs and coordinated 
> > > discarding of"
> > > -                         " RAM (e.g., virtio-mem) works, however, 
> > > malicious"
> > > -                         " guests can trigger pinning of more memory 
> > > than"
> > > -                         " intended via an IOMMU. It's possible to 
> > > mitigate "
> > > -                         " by setting/adjusting RLIMIT_MEMLOCK.");
> > > -    }
> > > -
> > > -    /*
> > > -     * Translation truncates length to the IOMMU page size,
> > > -     * check that it did not truncate too much.
> > > -     */
> > > -    if (len & iotlb->addr_mask) {
> > > -        error_report("iommu has granularity incompatible with target 
> > > AS");
> > > -        return false;
> > > -    }
> > > -
> > > -    if (vaddr) {
> > > -        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > -    }
> > > -
> > > -    if (ram_addr) {
> > > -        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > -    }
> > > -
> > > -    if (read_only) {
> > > -        *read_only = !writable || mr->readonly;
> > > -    }
> > > -
> > > -    return true;
> > > -}
> > > -
> > >  static void vfio_iommu_map_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > >  {
> > >      VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
> > > @@ -682,7 +596,8 @@ static void vfio_iommu_map_notify(IOMMUNotifier *n, 
> > > IOMMUTLBEntry *iotlb)
> > >      if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
> > >          bool read_only;
> > >
> > > -        if (!vfio_get_xlat_addr(iotlb, &vaddr, NULL, &read_only)) {
> > > +        if (!memory_get_xlat_addr(iotlb, &vaddr, NULL, &read_only,
> > > +                                  &address_space_memory)) {
> > >              goto out;
> > >          }
> > >          /*
> > > @@ -1359,7 +1274,8 @@ static void 
> > > vfio_iommu_map_dirty_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> > >      }
> > >
> > >      rcu_read_lock();
> > > -    if (vfio_get_xlat_addr(iotlb, NULL, &translated_addr, NULL)) {
> > > +    if (memory_get_xlat_addr(iotlb, NULL, &translated_addr, NULL,
> > > +                             &address_space_memory)) {
> > >          int ret;
> > >
> > >          ret = vfio_get_dirty_bitmap(container, iova, iotlb->addr_mask + 
> > > 1,
> > > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > > index bfb1de8eea..282de1d5ad 100644
> > > --- a/include/exec/memory.h
> > > +++ b/include/exec/memory.h
> > > @@ -713,6 +713,10 @@ void 
> > > ram_discard_manager_register_listener(RamDiscardManager *rdm,
> > >  void ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > >                                               RamDiscardListener *rdl);
> > >
> > > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > +                          ram_addr_t *ram_addr, bool *read_only,
> > > +                          AddressSpace *as);
> > > +
> > >  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> > >  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
> > >
> > > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > > index 7ba2048836..8586863ffa 100644
> > > --- a/softmmu/memory.c
> > > +++ b/softmmu/memory.c
> > > @@ -2121,6 +2121,90 @@ void 
> > > ram_discard_manager_unregister_listener(RamDiscardManager *rdm,
> > >      rdmc->unregister_listener(rdm, rdl);
> > >  }
> > >
> > > +/* Called with rcu_read_lock held.  */
> > > +bool memory_get_xlat_addr(IOMMUTLBEntry *iotlb, void **vaddr,
> > > +                          ram_addr_t *ram_addr, bool *read_only,
> > > +                          AddressSpace *as)
> > > +{
> > > +    MemoryRegion *mr;
> > > +    hwaddr xlat;
> > > +    hwaddr len = iotlb->addr_mask + 1;
> > > +    bool writable = iotlb->perm & IOMMU_WO;
> > > +
> > > +    /*
> > > +     * The IOMMU TLB entry we have just covers translation through
> > > +     * this IOMMU to its immediate target.  We need to translate
> > > +     * it the rest of the way through to memory.
> > > +     */
> > > +    mr = address_space_translate(as, iotlb->translated_addr, &xlat, &len,
> > > +                                 writable, MEMTXATTRS_UNSPECIFIED);
> > > +    if (!memory_region_is_ram(mr)) {
> > > +        error_report("iommu map to non memory area %" HWADDR_PRIx "", 
> > > xlat);
> > > +        return false;
> > > +    } else if (memory_region_has_ram_discard_manager(mr)) {
> > > +        RamDiscardManager *rdm = 
> > > memory_region_get_ram_discard_manager(mr);
> > > +        MemoryRegionSection tmp = {
> > > +            .mr = mr,
> > > +            .offset_within_region = xlat,
> > > +            .size = int128_make64(len),
> > > +        };
> > > +
> > > +        /*
> > > +         * Malicious VMs can map memory into the IOMMU, which is expected
> > > +         * to remain discarded. device will pin all pages, populating 
> > > memory.
> > > +         * Disallow that. vmstate priorities make sure any 
> > > RamDiscardManager
> > > +         * were already restored before IOMMUs are restored.
> > > +         */
> > > +        if (!ram_discard_manager_is_populated(rdm, &tmp)) {
> > > +            error_report("iommu map to discarded memory (e.g., unplugged 
> > > via"
> > > +                         " virtio-mem): %" HWADDR_PRIx "",
> > > +                         iotlb->translated_addr);
> > > +            return false;
> > > +        }
> > > +
> > > +        /*
> > > +         * Malicious VMs might trigger discarding of IOMMU-mapped 
> > > memory. The
> > > +         * pages will remain pinned inside device until unmapped, 
> > > resulting in a
> > > +         * higher memory consumption than expected. If memory would get
> > > +         * populated again later, there would be an inconsistency 
> > > between pages
> > > +         * pinned by device and pages seen by QEMU. This is the case 
> > > until
> > > +         * unmapped from the IOMMU (e.g., during device reset).
> > > +         *
> > > +         * With malicious guests, we really only care about pinning more 
> > > memory
> > > +         * than expected. RLIMIT_MEMLOCK set for the user/process can 
> > > never be
> > > +         * exceeded and can be used to mitigate this problem.
> > > +         */
> > > +        warn_report_once("Using device with vIOMMUs and coordinated 
> > > discarding"
> > > +                         " of RAM (e.g., virtio-mem) works, however, 
> > > malicious"
> > > +                         " guests can trigger pinning of more memory 
> > > than"
> > > +                         " intended via an IOMMU. It's possible to 
> > > mitigate "
> > > +                         " by setting/adjusting RLIMIT_MEMLOCK.");  
> >
> > Is this really fit to be in shared code?  Simply replacing "vfio" with
> > "device" for comments and warnings that are really of concern for a
> > specific use case doesn't look much better to me.
> >
> > I think translating an unpopulated address, as in the previous test
> > above, is generally invalid, but the comment is certainly trying to
> > frame the severity of this error relative to a specific use case.
> >
> > Here we're generating an unconditional warning, assuming that this code
> > path has been triggered by device code, for the condition of simply
> > asking for a translation to a MemoryRegion under discard manager
> > control?  Again, isn't that an action that has implications for a
> > specific use case of a device that supports pinning host memory?  
> 
> 
> Or can we rename the function to memory_get_xlat_addr_no_discard()?
> This looks more general and fit for the caller that doesn't want to
> map region that has a discard manager.

Is a guest restricted from mapping virtio-mem regions to a device?
AFAIK, this is something that a guest can do and we can't restrict them
from doing it, it's just that it opens some potential for malicious
activity that we rely on things like locked memory limits to keep from
getting out of hand.  Thanks,

Alex

> > Should the shared code be generating this warning, or could an optional
> > pointer arg be updated to indicate a translation to discard manager
> > controlled memory and this comment and warning should remain in the
> > caller?  Thanks,  
> 
> I think this should also work.
> 
> Thanks
> 
> >
> > Alex
> >  
> > > +    }
> > > +
> > > +    /*
> > > +     * Translation truncates length to the IOMMU page size,
> > > +     * check that it did not truncate too much.
> > > +     */
> > > +    if (len & iotlb->addr_mask) {
> > > +        error_report("iommu has granularity incompatible with target 
> > > AS");
> > > +        return false;
> > > +    }
> > > +
> > > +    if (vaddr) {
> > > +        *vaddr = memory_region_get_ram_ptr(mr) + xlat;
> > > +    }
> > > +
> > > +    if (ram_addr) {
> > > +        *ram_addr = memory_region_get_ram_addr(mr) + xlat;
> > > +    }
> > > +
> > > +    if (read_only) {
> > > +        *read_only = !writable || mr->readonly;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >  void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> > >  {
> > >      uint8_t mask = 1 << client;  
> >  
>