[PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Geert Uytterhoeven]

If iommu_ops.add_device() fails, iommu_ops.domain_free() is still
called, leading to a crash, as the domain was only partially
initialized:

    ipmmu-vmsa e67b0000.mmu: Cannot accommodate DMA translation for IOMMU page tables
    sata_rcar ee300000.sata: Unable to initialize IPMMU context
    iommu: Failed to add device ee300000.sata to group 0: -22
    Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038
    ...
    Call trace:
     ipmmu_domain_free+0x1c/0xa0
     iommu_group_release+0x48/0x68
     kobject_put+0x74/0xe8
     kobject_del.part.0+0x3c/0x50
     kobject_put+0x60/0xe8
     iommu_group_get_for_dev+0xa8/0x1f0
     ipmmu_add_device+0x1c/0x40
     of_iommu_configure+0x118/0x190

Fix this by checking if the domain's context already exists, before
trying to destroy it.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/iommu/ipmmu-vmsa.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index c4114b92652eb0c9..a8b2c649c1d1f1b9 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -498,6 +498,9 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 
 static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
 {
+	if (!domain->mmu)
+		return;
+
 	/*
 	 * Disable the context. Flush the TLB as required when modifying the
 	 * context registers.
-- 
2.17.1

Re: [PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Robin Murphy]

On 2018-11-07 1:18 pm, Geert Uytterhoeven wrote:
> If iommu_ops.add_device() fails, iommu_ops.domain_free() is still
> called, leading to a crash, as the domain was only partially
> initialized:
> 
>      ipmmu-vmsa e67b0000.mmu: Cannot accommodate DMA translation for IOMMU page tables
>      sata_rcar ee300000.sata: Unable to initialize IPMMU context
>      iommu: Failed to add device ee300000.sata to group 0: -22
>      Unable to handle kernel NULL pointer dereference at virtual address 0000000000000038
>      ...
>      Call trace:
>       ipmmu_domain_free+0x1c/0xa0
>       iommu_group_release+0x48/0x68
>       kobject_put+0x74/0xe8
>       kobject_del.part.0+0x3c/0x50
>       kobject_put+0x60/0xe8
>       iommu_group_get_for_dev+0xa8/0x1f0
>       ipmmu_add_device+0x1c/0x40
>       of_iommu_configure+0x118/0x190
> 
> Fix this by checking if the domain's context already exists, before
> trying to destroy it.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>   drivers/iommu/ipmmu-vmsa.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index c4114b92652eb0c9..a8b2c649c1d1f1b9 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -498,6 +498,9 @@ static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
>   
>   static void ipmmu_domain_destroy_context(struct ipmmu_vmsa_domain *domain)
>   {
> +	if (!domain->mmu)
> +		return;
> +
>   	/*
>   	 * Disable the context. Flush the TLB as required when modifying the
>   	 * context registers.
> 

Re: [PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Joerg Roedel]

On Wed, Nov 07, 2018 at 01:22:52PM +0000, Robin Murphy wrote:
> On 2018-11-07 1:18 pm, Geert Uytterhoeven wrote:
> > Fix this by checking if the domain's context already exists, before
> > trying to destroy it.
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Does this need a Fixes-tag? If so, which patch should be in that tag?


Thanks,

	Joerg

Re: [PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Geert Uytterhoeven]

Hi Jörg,

On Wed, Nov 7, 2018 at 4:34 PM Joerg Roedel <joro@8bytes.org> wrote:
> On Wed, Nov 07, 2018 at 01:22:52PM +0000, Robin Murphy wrote:
> > On 2018-11-07 1:18 pm, Geert Uytterhoeven wrote:
> > > Fix this by checking if the domain's context already exists, before
> > > trying to destroy it.
> >
> > Reviewed-by: Robin Murphy <robin.murphy@arm.com>
>
> Does this need a Fixes-tag? If so, which patch should be in that tag?

I think this bug has been present since the initial version of the driver, but
this failure mode has probably never been tested before.

It only got triggered by the combination of commits 6c2fb2ea76361da9
("of/device: Set bus DMA mask as appropriate") and b4ebe6063204da58
("dma-direct: implement complete bus_dma_mask handling"), which is being
fixed by "of/device: Really only set bus DMA mask when
appropriate" (https://patchwork.kernel.org/patch/10670177/).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Joerg Roedel]

On Wed, Nov 07, 2018 at 04:50:40PM +0100, Geert Uytterhoeven wrote:
> It only got triggered by the combination of commits 6c2fb2ea76361da9
> ("of/device: Set bus DMA mask as appropriate") and b4ebe6063204da58
> ("dma-direct: implement complete bus_dma_mask handling"), which is being
> fixed by "of/device: Really only set bus DMA mask when
> appropriate" (https://patchwork.kernel.org/patch/10670177/).

Okay, but the bug is triggered since 6c2fb2ea76361da9, so I took this
one for the fixes-tag.

Thanks,

	Joerg

Re: [PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Joerg Roedel]

On Wed, Nov 07, 2018 at 02:18:50PM +0100, Geert Uytterhoeven wrote:
 
> Fix this by checking if the domain's context already exists, before
> trying to destroy it.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/iommu/ipmmu-vmsa.c | 3 +++
>  1 file changed, 3 insertions(+)

Applied, thanks.

Re: [PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Robin Murphy]

On 2018-11-07 4:03 pm, Joerg Roedel wrote:
> On Wed, Nov 07, 2018 at 04:50:40PM +0100, Geert Uytterhoeven wrote:
>> It only got triggered by the combination of commits 6c2fb2ea76361da9
>> ("of/device: Set bus DMA mask as appropriate") and b4ebe6063204da58
>> ("dma-direct: implement complete bus_dma_mask handling"), which is being
>> fixed by "of/device: Really only set bus DMA mask when
>> appropriate" (https://patchwork.kernel.org/patch/10670177/).
> 
> Okay, but the bug is triggered since 6c2fb2ea76361da9, so I took this
> one for the fixes-tag.

FWIW it looks like it *has* always been possible to hit this crash by 
allocating a domain and freeing it again without attaching any devices, 
it's just highly improbable for any sane code to do that explicitly, so 
the real latent triggers are failure paths in external callers (which in 
this case are themselves only being reached thanks to my bug elsewhere).

Robin.

Re: [PATCH] iommu/ipmmu-vmsa: Fix crash on early domain free

[Joerg Roedel]

On Wed, Nov 07, 2018 at 04:17:09PM +0000, Robin Murphy wrote:
> FWIW it looks like it *has* always been possible to hit this crash by
> allocating a domain and freeing it again without attaching any devices, it's
> just highly improbable for any sane code to do that explicitly, so the real
> latent triggers are failure paths in external callers (which in this case
> are themselves only being reached thanks to my bug elsewhere).

Okay, given this, it probably makes more sense to change the fixes tag.
This is what I just committed:

Fixes: d25a2a16f0889 ('iommu: Add driver for Renesas VMSA-compatible IPMMU')

Regards,

	Joerg