|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors
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@xxxxxxxxxx>
Acked-by: Brian Woods <brian.woods@xxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> CC: Brian Woods <brian.woods@xxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |