Re: [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors

["Woods, Brian" <Brian.Woods@amd.com>]

On 11/21/18 7:21 AM, Andrew Cooper wrote:
> Most of these issues would be XSAs if these paths were accessible to guests.
> 
> First, override the {get,put}_gfn() helpers to use gfn_t, which was the
> original purpose of this patch.
> 
> guest_iommu_get_table_mfn() has two bugs.  First, it gets a ref on one gfn,
> and puts a ref for a different gfn.  This is only a latent bug for now, as we
> don't do per-gfn locking yet.  Next, the mfn return value is unsafe to use
> after put_gfn() is called, as the guest could have freed the page in the
> meantime.
> 
> In addition, get_gfn_from_base_reg() erroneously asserts that base_raw can't
> be 0, but it may legitimately be.  On top of that, the return value from
> guest_iommu_get_table_mfn() is passed into map_domain_page() before checking
> that it is a real mfn.
> 
> Most of the complexity here is inlining guest_iommu_get_table_mfn() and
> holding the gfn reference until the operation is complete.
> 
> Furthermore, guest_iommu_process_command() is altered to take a local copy of
> cmd_entry_t, rather than passing a pointer to guest controlled memory into
> each of the handling functions.  It is also modified to break on error rather
> than continue.  These changes are in line with the spec which states that the
> IOMMU will strictly read a command entry once, and will cease processing if an
> error is encountered.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Brian Woods <brian.woods@amd.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>
> 
> This patch my no means indicates that the code is ready for production use.
> ---
>   xen/drivers/passthrough/amd/iommu_guest.c | 224 +++++++++++++++++++-----------
>   1 file changed, 146 insertions(+), 78 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
> index 96175bb..03ca0cf 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -21,6 +21,13 @@
>   #include <asm/amd-iommu.h>
>   #include <asm/hvm/svm/amd-iommu-proto.h>
>   
> +/* Override {get,put}_gfn to work with gfn_t */
> +#undef get_gfn
> +#define get_gfn(d, g, t) get_gfn_type(d, gfn_x(g), t, P2M_ALLOC)
> +#undef get_gfn_query
> +#define get_gfn_query(d, g, t) get_gfn_type(d, gfn_x(g), t, 0)
> +#undef put_gfn
> +#define put_gfn(d, g) __put_gfn(p2m_get_hostp2m(d), gfn_x(g))
>   
>   #define IOMMU_MMIO_SIZE                         0x8000
>   #define IOMMU_MMIO_PAGE_NR                      0x8
> @@ -117,13 +124,6 @@ static unsigned int host_domid(struct domain *d, uint64_t g_domid)
>       return d->domain_id;
>   }
>   
> -static unsigned long get_gfn_from_base_reg(uint64_t base_raw)
> -{
> -    base_raw &= PADDR_MASK;
> -    ASSERT ( base_raw != 0 );
> -    return base_raw >> PAGE_SHIFT;
> -}
> -
>   static void guest_iommu_deliver_msi(struct domain *d)
>   {
>       uint8_t vector, dest, dest_mode, delivery_mode, trig_mode;
> @@ -138,23 +138,6 @@ static void guest_iommu_deliver_msi(struct domain *d)
>       vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
>   }
>   
> -static unsigned long guest_iommu_get_table_mfn(struct domain *d,
> -                                               uint64_t base_raw,
> -                                               unsigned int entry_size,
> -                                               unsigned int pos)
> -{
> -    unsigned long idx, gfn, mfn;
> -    p2m_type_t p2mt;
> -
> -    gfn = get_gfn_from_base_reg(base_raw);
> -    idx = (pos * entry_size) >> PAGE_SHIFT;
> -
> -    mfn = mfn_x(get_gfn(d, gfn + idx, &p2mt));
> -    put_gfn(d, gfn);
> -
> -    return mfn;
> -}
> -
>   static void guest_iommu_enable_dev_table(struct guest_iommu *iommu)
>   {
>       uint32_t length_raw = get_field_from_reg_u32(iommu->dev_table.reg_base.lo,
> @@ -176,7 +159,10 @@ static void guest_iommu_enable_ring_buffer(struct guest_iommu *iommu,
>   void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
>   {
>       uint16_t gdev_id;
> -    unsigned long mfn, tail, head;
> +    unsigned long tail, head;
> +    mfn_t mfn;
> +    gfn_t gfn;
> +    p2m_type_t p2mt;
>       ppr_entry_t *log, *log_base;
>       struct guest_iommu *iommu;
>   
> @@ -197,11 +183,24 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
>           return;
>       }
>   
> -    mfn = guest_iommu_get_table_mfn(d, reg_to_u64(iommu->ppr_log.reg_base),
> -                                    sizeof(ppr_entry_t), tail);
> -    ASSERT(mfn_valid(_mfn(mfn)));
> +    gfn = _gfn(PFN_DOWN(reg_to_u64(iommu->ppr_log.reg_base)) +
> +               PFN_DOWN(tail * sizeof(*log)));
>   
> -    log_base = map_domain_page(_mfn(mfn));
> +    mfn = get_gfn(d, gfn, &p2mt);
> +    if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(p2mt) )
> +    {
> +        AMD_IOMMU_DEBUG(
> +            "Error: guest iommu ppr log bad gfn %"PRI_gfn", type %u, mfn %"
> +            PRI_mfn", reg_base %#"PRIx64", tail %#lx\n",
> +            gfn_x(gfn), p2mt, mfn_x(mfn),
> +            reg_to_u64(iommu->ppr_log.reg_base), tail);
> +        guest_iommu_disable(iommu);
> +        goto out;
> +    }
> +
> +    ASSERT(mfn_valid(mfn));
> +
> +    log_base = map_domain_page(mfn);
>       log = log_base + tail % (PAGE_SIZE / sizeof(ppr_entry_t));
>   
>       /* Convert physical device id back into virtual device id */
> @@ -220,12 +219,18 @@ void guest_iommu_add_ppr_log(struct domain *d, u32 entry[])
>       unmap_domain_page(log_base);
>   
>       guest_iommu_deliver_msi(d);
> +
> +out:
> +    put_gfn(d, gfn);
>   }
>   
>   void guest_iommu_add_event_log(struct domain *d, u32 entry[])
>   {
>       uint16_t dev_id;
> -    unsigned long mfn, tail, head;
> +    unsigned long tail, head;
> +    mfn_t mfn;
> +    gfn_t gfn;
> +    p2m_type_t p2mt;
>       event_entry_t *log, *log_base;
>       struct guest_iommu *iommu;
>   
> @@ -246,11 +251,24 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
>           return;
>       }
>   
> -    mfn = guest_iommu_get_table_mfn(d, reg_to_u64(iommu->event_log.reg_base),
> -                                    sizeof(event_entry_t), tail);
> -    ASSERT(mfn_valid(_mfn(mfn)));
> +    gfn = _gfn(PFN_DOWN(reg_to_u64(iommu->event_log.reg_base)) +
> +               PFN_DOWN(tail * sizeof(*log)));
> +
> +    mfn = get_gfn(d, gfn, &p2mt);
> +    if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(p2mt) )
> +    {
> +        AMD_IOMMU_DEBUG(
> +            "Error: guest iommu event log bad gfn %"PRI_gfn", type %u, mfn %"
> +            PRI_mfn", reg_base %#"PRIx64", tail %#lx\n",
> +            gfn_x(gfn), p2mt, mfn_x(mfn),
> +            reg_to_u64(iommu->ppr_log.reg_base), tail);
> +        guest_iommu_disable(iommu);
> +        goto out;
> +    }
> +
> +    ASSERT(mfn_valid(mfn));
>   
> -    log_base = map_domain_page(_mfn(mfn));
> +    log_base = map_domain_page(mfn);
>       log = log_base + tail % (PAGE_SIZE / sizeof(event_entry_t));
>   
>       /* re-write physical device id into virtual device id */
> @@ -269,6 +287,9 @@ void guest_iommu_add_event_log(struct domain *d, u32 entry[])
>       unmap_domain_page(log_base);
>   
>       guest_iommu_deliver_msi(d);
> +
> +out:
> +    put_gfn(d, gfn);
>   }
>   
>   static int do_complete_ppr_request(struct domain *d, cmd_entry_t *cmd)
> @@ -346,10 +367,8 @@ static int do_invalidate_iotlb_pages(struct domain *d, cmd_entry_t *cmd)
>   
>   static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
>   {
> -    bool_t com_wait_int_en, com_wait_int, i, s;
> +    bool com_wait_int_en, com_wait_int, i, s;
>       struct guest_iommu *iommu;
> -    unsigned long gfn;
> -    p2m_type_t p2mt;
>   
>       iommu = domain_iommu(d);
>   
> @@ -362,7 +381,10 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
>       if ( s )
>       {
>           uint64_t gaddr_lo, gaddr_hi, gaddr_64, data;
> -        void *vaddr;
> +        mfn_t mfn;
> +        gfn_t gfn;
> +        p2m_type_t p2mt;
> +        uint64_t *ptr;
>   
>           data = (uint64_t)cmd->data[3] << 32 | cmd->data[2];
>           gaddr_lo = get_field_from_reg_u32(cmd->data[0],
> @@ -374,13 +396,24 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
>   
>           gaddr_64 = (gaddr_hi << 32) | (gaddr_lo << 3);
>   
> -        gfn = gaddr_64 >> PAGE_SHIFT;
> -        vaddr = map_domain_page(get_gfn(d, gfn ,&p2mt));
> -        put_gfn(d, gfn);
> +        gfn = _gfn(gaddr_64 >> PAGE_SHIFT);
> +        mfn = get_gfn(d, gfn, &p2mt);
>   
> -        write_u64_atomic((uint64_t *)(vaddr + (gaddr_64 & (PAGE_SIZE-1))),
> -                         data);
> -        unmap_domain_page(vaddr);
> +        if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(p2mt) )
> +        {
> +            /* XXX - What to do here, error wise? */
> +            guest_iommu_disable(iommu);
> +            put_gfn(d, gfn);
> +
> +            return 0;
> +        }
> +
> +        ptr = map_domain_page(mfn) + (gaddr_64 & ~PAGE_MASK);
> +
> +        write_u64_atomic(ptr, data);
> +        unmap_domain_page(ptr);
> +
> +        put_gfn(d, gfn);
>       }
>   
>       com_wait_int_en = iommu_get_bit(iommu->reg_ctrl.lo,
> @@ -400,9 +433,10 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>       dev_entry_t *gdte, *mdte, *dte_base;
>       struct amd_iommu *iommu = NULL;
>       struct guest_iommu *g_iommu;
> -    uint64_t gcr3_gfn, gcr3_mfn;
> +    mfn_t dte_mfn, gcr3_mfn;
> +    gfn_t dte_gfn, gcr3_gfn;
>       uint8_t glx, gv;
> -    unsigned long dte_mfn, flags;
> +    unsigned long flags;
>       p2m_type_t p2mt;
>   
>       g_iommu = domain_iommu(d);
> @@ -417,35 +451,49 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>       if ( (gbdf * sizeof(dev_entry_t)) > g_iommu->dev_table.size )
>           return 0;
>   
> -    dte_mfn = guest_iommu_get_table_mfn(d,
> -                                        reg_to_u64(g_iommu->dev_table.reg_base),
> -                                        sizeof(dev_entry_t), gbdf);
> -    ASSERT(mfn_valid(_mfn(dte_mfn)));
> +    dte_gfn = _gfn(PFN_DOWN(reg_to_u64(g_iommu->dev_table.reg_base)) +
> +                   PFN_DOWN(gbdf * sizeof(*gdte)));
> +    dte_mfn = get_gfn(d, dte_gfn, &p2mt);
> +
> +    if ( mfn_eq(dte_mfn, INVALID_MFN) || !p2m_is_ram(p2mt) )
> +    {
> +        put_gfn(d, dte_gfn);
> +        return 0;
> +    }
> +
> +    ASSERT(mfn_valid(dte_mfn));
>   
>       /* Read guest dte information */
> -    dte_base = map_domain_page(_mfn(dte_mfn));
> +    dte_base = map_domain_page(dte_mfn);
>   
>       gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
>   
>       gdom_id  = get_domid_from_dte(gdte);
> -    gcr3_gfn = get_guest_cr3_from_dte(gdte);
> +    gcr3_gfn = _gfn(get_guest_cr3_from_dte(gdte));
>       glx      = get_glx_from_dte(gdte);
>       gv       = get_gv_from_dte(gdte);
>   
>       unmap_domain_page(dte_base);
> +    put_gfn(d, dte_gfn);
>   
>       /* Do not update host dte before gcr3 has been set */
> -    if ( gcr3_gfn == 0 )
> +    if ( gfn_x(gcr3_gfn) == 0 )
>           return 0;
>   
> -    gcr3_mfn = mfn_x(get_gfn(d, gcr3_gfn, &p2mt));
> -    put_gfn(d, gcr3_gfn);
> +    gcr3_mfn = get_gfn(d, gcr3_gfn, &p2mt);
>   
> -    ASSERT(mfn_valid(_mfn(gcr3_mfn)));
> +    if ( mfn_eq(gcr3_mfn, INVALID_MFN) || !p2m_is_ram(p2mt) )
> +    {
> +        put_gfn(d, gcr3_gfn);
> +        return 0;
> +    }
> +
> +    ASSERT(mfn_valid(gcr3_mfn));
>   
>       iommu = find_iommu_for_device(0, mbdf);
>       if ( !iommu )
>       {
> +        put_gfn(d, gcr3_gfn);
>           AMD_IOMMU_DEBUG("%s: Fail to find iommu for bdf %x!\n",
>                           __func__, mbdf);
>           return -ENODEV;
> @@ -458,18 +506,19 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
>   
>       spin_lock_irqsave(&iommu->lock, flags);
>       iommu_dte_set_guest_cr3((u32 *)mdte, hdom_id,
> -                            gcr3_mfn << PAGE_SHIFT, gv, glx);
> +                            mfn_to_maddr(gcr3_mfn), gv, glx);
>   
>       amd_iommu_flush_device(iommu, req_id);
>       spin_unlock_irqrestore(&iommu->lock, flags);
>   
> +    put_gfn(d, gcr3_gfn);
> +
>       return 0;
>   }
>   
>   static void guest_iommu_process_command(unsigned long _d)
>   {
> -    unsigned long opcode, tail, head, entries_per_page, cmd_mfn;
> -    cmd_entry_t *cmd, *cmd_base;
> +    unsigned long tail, head;
>       struct domain *d = (struct domain *)_d;
>       struct guest_iommu *iommu;
>   
> @@ -493,56 +542,75 @@ static void guest_iommu_process_command(unsigned long _d)
>           return;
>       }
>   
> -    entries_per_page = PAGE_SIZE / sizeof(cmd_entry_t);
> -
>       while ( head != tail )
>       {
> +        mfn_t mfn;
> +        gfn_t gfn;
> +        p2m_type_t p2mt;
> +        cmd_entry_t cmd, *ptr;
>           int ret = 0;
> +        unsigned int opcode;
> +
> +        gfn = _gfn(PFN_DOWN(reg_to_u64(iommu->cmd_buffer.reg_base)) +
> +                   PFN_DOWN(head * sizeof(cmd)));
>   
> -        cmd_mfn = guest_iommu_get_table_mfn(d,
> -                                            reg_to_u64(iommu->cmd_buffer.reg_base),
> -                                            sizeof(cmd_entry_t), head);
> -        ASSERT(mfn_valid(_mfn(cmd_mfn)));
> +        mfn = get_gfn(d, gfn, &p2mt);
> +        if ( mfn_eq(mfn, INVALID_MFN) || !p2m_is_ram(p2mt) )
> +        {
> +            AMD_IOMMU_DEBUG(
> +                "Error: guest iommu cmd buffer bad gfn %"PRI_gfn", type %u, mfn %"
> +                PRI_mfn", reg_base %#"PRIx64", head %#lx\n",
> +                gfn_x(gfn), p2mt, mfn_x(mfn),
> +                reg_to_u64(iommu->cmd_buffer.reg_base), head);
> +            put_gfn(d, gfn);
> +            guest_iommu_disable(iommu);
> +            return;
> +        }
>   
> -        cmd_base = map_domain_page(_mfn(cmd_mfn));
> -        cmd = cmd_base + head % entries_per_page;
> +        ptr = map_domain_page(mfn) + head % (PAGE_SIZE / sizeof(cmd_entry_t));
> +        memcpy(&cmd, ptr, sizeof(cmd));
> +        unmap_domain_page(ptr);
> +        put_gfn(d, gfn);
>   
> -        opcode = get_field_from_reg_u32(cmd->data[1],
> +        opcode = get_field_from_reg_u32(cmd.data[1],
>                                           IOMMU_CMD_OPCODE_MASK,
>                                           IOMMU_CMD_OPCODE_SHIFT);
>           switch ( opcode )
>           {
>           case IOMMU_CMD_COMPLETION_WAIT:
> -            ret = do_completion_wait(d, cmd);
> +            ret = do_completion_wait(d, &cmd);
>               break;
>           case IOMMU_CMD_INVALIDATE_DEVTAB_ENTRY:
> -            ret = do_invalidate_dte(d, cmd);
> +            ret = do_invalidate_dte(d, &cmd);
>               break;
>           case IOMMU_CMD_INVALIDATE_IOMMU_PAGES:
> -            ret = do_invalidate_pages(d, cmd);
> +            ret = do_invalidate_pages(d, &cmd);
>               break;
>           case IOMMU_CMD_INVALIDATE_IOTLB_PAGES:
> -            ret = do_invalidate_iotlb_pages(d, cmd);
> +            ret = do_invalidate_iotlb_pages(d, &cmd);
>               break;
>           case IOMMU_CMD_INVALIDATE_INT_TABLE:
>               break;
>           case IOMMU_CMD_COMPLETE_PPR_REQUEST:
> -            ret = do_complete_ppr_request(d, cmd);
> +            ret = do_complete_ppr_request(d, &cmd);
>               break;
>           case IOMMU_CMD_INVALIDATE_IOMMU_ALL:
> -            ret = do_invalidate_all(d, cmd);
> +            ret = do_invalidate_all(d, &cmd);
>               break;
>           default:
> -            AMD_IOMMU_DEBUG("CMD: Unknown command cmd_type = %lx "
> +            AMD_IOMMU_DEBUG("CMD: Unknown command cmd_type = %#x "
>                               "head = %ld\n", opcode, head);
> +            ret = -EINVAL;
>               break;
>           }
>   
> -        unmap_domain_page(cmd_base);
>           if ( ++head >= iommu->cmd_buffer.entries )
>               head = 0;
>           if ( ret )
> +        {
>               guest_iommu_disable(iommu);
> +            break;
> +        }
>       }
>   
>       /* Now shift cmd buffer head pointer */
> @@ -818,10 +886,10 @@ int guest_iommu_set_base(struct domain *d, uint64_t base)
>   
>       for ( int i = 0; i < IOMMU_MMIO_PAGE_NR; i++ )
>       {
> -        unsigned long gfn = base + i;
> +        gfn_t gfn = _gfn(base + i);
>   
>           get_gfn_query(d, gfn, &t);
> -        p2m_change_type_one(d, gfn, t, p2m_mmio_dm);
> +        p2m_change_type_one(d, gfn_x(gfn), t, p2m_mmio_dm);
>           put_gfn(d, gfn);
>       }
>   
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel