| Message ID | 20181107131850.11584-1-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | iommu/ipmmu-vmsa: Fix crash on early domain free | expand |
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. >
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
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
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
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.
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.
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
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.
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(+)