Hi Paul, On 04/01/2021 09:28, Paul Durrant wrote:
-----Original Message----- From: Julien Grall <jul...@xen.org> Sent: 22 December 2020 15:44 To: xen-devel@lists.xenproject.org Cc: hongy...@amazon.co.uk; Julien Grall <jgr...@amazon.com>; Jan Beulich <jbeul...@suse.com>; Paul Durrant <p...@xen.org> Subject: [PATCH for-4.15 1/4] xen/iommu: Check if the IOMMU was initialized before tearing downFrom: Julien Grall <jgr...@amazon.com> 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 <jgr...@amazon.com> --- 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.
I originally resolved this way to avoid arch_iommu_domain_init() and iommu_teardown() to be called when the failure may happen before iommu_domain_init() is called. IOW, the lists/locks would be unitialized.
After discussing with Jan, it turns out that we could check whether the list was initialized or not. So I have reworked the code to now call arch_iommu_domain_init() unconditionally and move the ops check in iommu_teardown().
Cheers, -- Julien Grall
