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
Acked-by: Brian Woods
> ---
> CC: Jan Beulich
> CC: Wei Liu
> CC: Roger Pau Monné
> CC: Suravee Suthikulpanit
> CC: Brian Woods
>
> 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
> #include
>
> +/* 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),
Re: [Xen-devel] [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors
>>> On 23.11.18 at 17:03, wrote: > On 23/11/2018 08:23, Jan Beulich wrote: > On 22.11.18 at 18:46, wrote: >>> On 22/11/2018 14:51, Jan Beulich wrote: > @@ -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: Please indent by at least one blank (same further down). >>> I thought you've said that you're no longer using an obsolete version of >>> diff. >> I don't think I've ever said so, and I don't think it matters much what >> I personally use. > > The only reason we have this quirk of a coding style is because you > insisted when David was putting together CODING_STYLE, and the reason > you gave was to work around versions of `diff -u` which mistook the case > label for the function name when creating the unified header. v1 of > David's patch very specifically didn't have the one space, because he > was actually trying to get rid of that, as it was (and still is) > completely unexpected for new developers, and a frequent source of curt > review responses. Personally I find it undesirable to have anything other that file scope things to start in column 1. Of course, as you're likely aware, I'm an opponent of labels and goto-s altogether, and probably the former dislike is sort of a result of the latter. > If this is no longer a concern, can I suggest we update CODING_STYLE. Even the version of diff on my main most modern system (3.6) gets this wrong, so I'd prefer if we could stick to indented labels. I continue to be of the opinion that the use of git should not be a requirement for developers; plain diff and patch ought to suffice for people to produce acceptable patches. I'd like to note though that ./CODING_STYLE says nothing either way afaics, so I also have no basis to _insist_ on this style. I've just sent a patch to rectify this. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors
On 23/11/2018 08:23, Jan Beulich wrote: On 22.11.18 at 18:46, wrote: >> On 22/11/2018 14:51, Jan Beulich wrote: @@ -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: >>> Please indent by at least one blank (same further down). >> I thought you've said that you're no longer using an obsolete version of >> diff. > I don't think I've ever said so, and I don't think it matters much what > I personally use. The only reason we have this quirk of a coding style is because you insisted when David was putting together CODING_STYLE, and the reason you gave was to work around versions of `diff -u` which mistook the case label for the function name when creating the unified header. v1 of David's patch very specifically didn't have the one space, because he was actually trying to get rid of that, as it was (and still is) completely unexpected for new developers, and a frequent source of curt review responses. If this is no longer a concern, can I suggest we update CODING_STYLE. This is one particular divergence from more normal styles which doesn't add to code clarity in any way I can spot. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors
>>> On 22.11.18 at 18:46, wrote: > On 22/11/2018 14:51, Jan Beulich wrote: >>> @@ -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: >> Please indent by at least one blank (same further down). > > I thought you've said that you're no longer using an obsolete version of > diff. I don't think I've ever said so, and I don't think it matters much what I personally use. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors
On 22/11/2018 14:51, Jan Beulich wrote: > >> @@ -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: > Please indent by at least one blank (same further down). I thought you've said that you're no longer using an obsolete version of diff. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors
>>> On 21.11.18 at 14:21, 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
Looks all plausible, but I haven't looked closely enough to give
an R-b and an A-b would be meaningless. One cosmetic remark
though:
> @@ -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:
Please indent by at least one blank (same further down).
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 03/14] AMD/IOMMU: Fix multiple reference counting errors
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
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Suravee Suthikulpanit
CC: Brian Woods
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
#include
+/* 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
