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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.11.18 at 14:21, <andrew.cooper3@xxxxxxxxxx> 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>

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel