RE: [PATCH for-4.15 1/4] xen/iommu: Check if the IOMMU was initialized before tearing down

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: 22 December 2020 15:44
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: hongyxia@xxxxxxxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; Jan Beulich 
> <jbeulich@xxxxxxxx>; Paul
> Durrant <paul@xxxxxxx>
> Subject: [PATCH for-4.15 1/4] xen/iommu: Check if the IOMMU was initialized 
> before tearing down
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> is_iommu_enabled() will return true even if the IOMMU has not been
> initialized (e.g. the ops are not set).
> 
> In the case of an early failure in arch_domain_init(), the function
> iommu_destroy_domain() will be called even if the IOMMU is initialized.
> 
> This will result to dereference the ops which will be NULL and an host
> crash.
> 
> Fix the issue by checking that ops has been set before accessing it.
> Note that we are assuming that arch_iommu_domain_init() will cleanup an
> intermediate failure if it failed.
> 
> Fixes: 71e617a6b8f6 ("use is_iommu_enabled() where appropriate...")
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
>  xen/drivers/passthrough/iommu.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 2358b6eb09f4..f976d5a0b0a5 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -226,7 +226,15 @@ static void iommu_teardown(struct domain *d)
> 
>  void iommu_domain_destroy(struct domain *d)
>  {
> -    if ( !is_iommu_enabled(d) )
> +    struct domain_iommu *hd = dom_iommu(d);
> +
> +    /*
> +     * In case of failure during the domain construction, it would be
> +     * possible to reach this function with the IOMMU enabled but not
> +     * yet initialized. We assume that hd->platforms will be non-NULL as
> +     * soon as we start to initialize the IOMMU.
> +     */
> +    if ( !is_iommu_enabled(d) || !hd->platform_ops )
>          return;

TBH, this seems like the wrong way to fix it. The ops dereference is done in 
iommu_teardown() so that ought to be doing the check,
but given it is single line function then it could be inlined at the same time. 
That said, I think arch_iommu_domain_destroy() needs
to be call unconditionally as it arch_iommu_domain_init() is called before the 
ops pointer is set.

  Paul

> 
>      iommu_teardown(d);
> --
> 2.17.1