| Message ID | 20201119165846.34180-1-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Check return of __iommu_attach_device() | expand |
On Thu, Nov 19, 2020 at 04:58:46PM +0000, Shameer Kolothum wrote: > Currently iommu_create_device_direct_mappings() is called > without checking the return of __iommu_attach_device(). This > may result in failures in iommu driver if dev attach returns > error. > > Fixes: ce574c27ae27("iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device()") > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > --- > Crash log: > [ 31.353605] hns3 0000:7d:00.3: Adding to iommu group 10 > [ 31.358822] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018 > [ 31.367567] Mem abort info: > [ 31.370350] ESR = 0x96000004 > [ 31.373391] EC = 0x25: DABT (current EL), IL = 32 bits > [ 31.378680] SET = 0, FnV = 0 > [ 31.381720] EA = 0, S1PTW = 0 > [ 31.384847] Data abort info: > [ 31.387716] ISV = 0, ISS = 0x00000004 > [ 31.391535] CM = 0, WnR = 0 > [ 31.394491] [0000000000000018] user address but active_mm is swapper > [ 31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP > [ 31.406365] Modules linked in: > [ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted 5.10.0-rc4-00008-gdd5aba9d719-dirty #79 > [ 31.417980] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020 > [ 31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--) > [ 31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0 > [ 31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0 > [ 31.443190] sp : ffff80001043b4e0 > ... > [ 31.531175] Call trace: > [ 31.533613] arm_smmu_tlb_inv_range+0x178/0x1f0 > [ 31.538122] arm_smmu_iotlb_sync+0x2c/0x38 > [ 31.542200] iommu_unmap+0x60/0x90 > [ 31.545585] __iommu_map+0x110/0x1f0 > [ 31.549144] iommu_create_device_direct_mappings.isra.34+0x1ac/0x250 > [ 31.555468] iommu_probe_device+0x6c/0x110 > [ 31.559551] iort_iommu_configure_id+0x114/0x218 > [ 31.564148] acpi_dma_configure_id+0x94/0xe0 > [ 31.568402] pci_dma_configure+0xc8/0xf0 > [ 31.572310] really_probe+0xd4/0x3e0 > [ 31.575871] driver_probe_device+0x5c/0xc0 > > --- > drivers/iommu/iommu.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index b53446bb8c6b..0f4dc25d46c9 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev) > */ > iommu_alloc_default_domain(group, dev); > > - if (group->default_domain) > + if (group->default_domain) { > ret = __iommu_attach_device(group->default_domain, dev); > + if (ret) { > + iommu_group_put(group); > + goto err_release; > + } > + } This looks sensible to me, but what I don't understand is where that NULL pointer is coming from in the first place. iommu_map() operates on the domain, so why does it matter if the attach fails? What is being accessed at arm_smmu_tlb_inv_range+0x178/0x1f0 ? Will
On 2020-11-20 11:15, Will Deacon wrote: > On Thu, Nov 19, 2020 at 04:58:46PM +0000, Shameer Kolothum wrote: >> Currently iommu_create_device_direct_mappings() is called >> without checking the return of __iommu_attach_device(). This >> may result in failures in iommu driver if dev attach returns >> error. >> >> Fixes: ce574c27ae27("iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device()") >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> >> --- >> Crash log: >> [ 31.353605] hns3 0000:7d:00.3: Adding to iommu group 10 >> [ 31.358822] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018 >> [ 31.367567] Mem abort info: >> [ 31.370350] ESR = 0x96000004 >> [ 31.373391] EC = 0x25: DABT (current EL), IL = 32 bits >> [ 31.378680] SET = 0, FnV = 0 >> [ 31.381720] EA = 0, S1PTW = 0 >> [ 31.384847] Data abort info: >> [ 31.387716] ISV = 0, ISS = 0x00000004 >> [ 31.391535] CM = 0, WnR = 0 >> [ 31.394491] [0000000000000018] user address but active_mm is swapper >> [ 31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP >> [ 31.406365] Modules linked in: >> [ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted 5.10.0-rc4-00008-gdd5aba9d719-dirty #79 >> [ 31.417980] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020 >> [ 31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--) >> [ 31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0 >> [ 31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0 >> [ 31.443190] sp : ffff80001043b4e0 >> ... >> [ 31.531175] Call trace: >> [ 31.533613] arm_smmu_tlb_inv_range+0x178/0x1f0 >> [ 31.538122] arm_smmu_iotlb_sync+0x2c/0x38 >> [ 31.542200] iommu_unmap+0x60/0x90 >> [ 31.545585] __iommu_map+0x110/0x1f0 >> [ 31.549144] iommu_create_device_direct_mappings.isra.34+0x1ac/0x250 >> [ 31.555468] iommu_probe_device+0x6c/0x110 >> [ 31.559551] iort_iommu_configure_id+0x114/0x218 >> [ 31.564148] acpi_dma_configure_id+0x94/0xe0 >> [ 31.568402] pci_dma_configure+0xc8/0xf0 >> [ 31.572310] really_probe+0xd4/0x3e0 >> [ 31.575871] driver_probe_device+0x5c/0xc0 >> >> --- >> drivers/iommu/iommu.c | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index b53446bb8c6b..0f4dc25d46c9 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev) >> */ >> iommu_alloc_default_domain(group, dev); >> >> - if (group->default_domain) >> + if (group->default_domain) { >> ret = __iommu_attach_device(group->default_domain, dev); >> + if (ret) { >> + iommu_group_put(group); >> + goto err_release; >> + } >> + } > > This looks sensible to me, but what I don't understand is where that > NULL pointer is coming from in the first place. iommu_map() operates > on the domain, so why does it matter if the attach fails? What is being > accessed at arm_smmu_tlb_inv_range+0x178/0x1f0 ? Probably because the domain is a hollow fake until the first successful attach - even TLB maintenance depends on having decided a pagetable format. Robin.
> -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@arm.com] > Sent: 20 November 2020 14:07 > To: Will Deacon <will@kernel.org>; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com> > Cc: linux-arm-kernel@lists.infradead.org; iommu@lists.linux-foundation.org; > Linuxarm <linuxarm@huawei.com> > Subject: Re: [PATCH] iommu: Check return of __iommu_attach_device() > > On 2020-11-20 11:15, Will Deacon wrote: > > On Thu, Nov 19, 2020 at 04:58:46PM +0000, Shameer Kolothum wrote: > >> Currently iommu_create_device_direct_mappings() is called > >> without checking the return of __iommu_attach_device(). This > >> may result in failures in iommu driver if dev attach returns > >> error. > >> > >> Fixes: ce574c27ae27("iommu: Move > iommu_group_create_direct_mappings() out of iommu_group_add_device()") > >> Signed-off-by: Shameer Kolothum > <shameerali.kolothum.thodi@huawei.com> > >> --- > >> Crash log: > >> [ 31.353605] hns3 0000:7d:00.3: Adding to iommu group 10 > >> [ 31.358822] Unable to handle kernel NULL pointer dereference at virtual > address 0000000000000018 > >> [ 31.367567] Mem abort info: > >> [ 31.370350] ESR = 0x96000004 > >> [ 31.373391] EC = 0x25: DABT (current EL), IL = 32 bits > >> [ 31.378680] SET = 0, FnV = 0 > >> [ 31.381720] EA = 0, S1PTW = 0 > >> [ 31.384847] Data abort info: > >> [ 31.387716] ISV = 0, ISS = 0x00000004 > >> [ 31.391535] CM = 0, WnR = 0 > >> [ 31.394491] [0000000000000018] user address but active_mm is > swapper > >> [ 31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP > >> [ 31.406365] Modules linked in: > >> [ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted > 5.10.0-rc4-00008-gdd5aba9d719-dirty #79 > >> [ 31.417980] Hardware name: Huawei TaiShan 200 (Model > 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020 > >> [ 31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--) > >> [ 31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0 > >> [ 31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0 > >> [ 31.443190] sp : ffff80001043b4e0 > >> ... > >> [ 31.531175] Call trace: > >> [ 31.533613] arm_smmu_tlb_inv_range+0x178/0x1f0 > >> [ 31.538122] arm_smmu_iotlb_sync+0x2c/0x38 > >> [ 31.542200] iommu_unmap+0x60/0x90 > >> [ 31.545585] __iommu_map+0x110/0x1f0 > >> [ 31.549144] > iommu_create_device_direct_mappings.isra.34+0x1ac/0x250 > >> [ 31.555468] iommu_probe_device+0x6c/0x110 > >> [ 31.559551] iort_iommu_configure_id+0x114/0x218 > >> [ 31.564148] acpi_dma_configure_id+0x94/0xe0 > >> [ 31.568402] pci_dma_configure+0xc8/0xf0 > >> [ 31.572310] really_probe+0xd4/0x3e0 > >> [ 31.575871] driver_probe_device+0x5c/0xc0 > >> > >> --- > >> drivers/iommu/iommu.c | 10 ++++++---- > >> 1 file changed, 6 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > >> index b53446bb8c6b..0f4dc25d46c9 100644 > >> --- a/drivers/iommu/iommu.c > >> +++ b/drivers/iommu/iommu.c > >> @@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev) > >> */ > >> iommu_alloc_default_domain(group, dev); > >> > >> - if (group->default_domain) > >> + if (group->default_domain) { > >> ret = __iommu_attach_device(group->default_domain, dev); > >> + if (ret) { > >> + iommu_group_put(group); > >> + goto err_release; > >> + } > >> + } > > > > This looks sensible to me, but what I don't understand is where that > > NULL pointer is coming from in the first place. iommu_map() operates > > on the domain, so why does it matter if the attach fails? What is being > > accessed at arm_smmu_tlb_inv_range+0x178/0x1f0 ? > > Probably because the domain is a hollow fake until the first successful > attach - even TLB maintenance depends on having decided a pagetable format. I think, in this particular instance, what happens is, dev reports RMR regions (IOMMU_RESV_DIRECT) but attach_dev() fails early without setting, smmu_domain->smmu = smmu. iommu_probe_device() __iommu_attach_dev() -->return err, but carries on. iommu_create_device_direct_mappings() iommu_get_resv_regions() --> dev has IOMMU_RESV_DIRECT regions iommu_map() __iommu_map() arm_smmu_map() -->return err iommu_unmap() --> unroll on map failure __iommu_unmap() --> size is zero. So returns. iommu_iotlb_sync() arm_smmu_iotlb_sync() arm_smmu_tlb_inv_range() --> smmu is NULL Thanks, Shameer
On Thu, 19 Nov 2020 16:58:46 +0000, Shameer Kolothum wrote: > Currently iommu_create_device_direct_mappings() is called > without checking the return of __iommu_attach_device(). This > may result in failures in iommu driver if dev attach returns > error. Applied to arm64 (for-next/iommu/fixes), thanks! [1/1] iommu: Check return of __iommu_attach_device() https://git.kernel.org/arm64/c/77c38c8cf52e Cheers,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index b53446bb8c6b..0f4dc25d46c9 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -264,16 +264,18 @@ int iommu_probe_device(struct device *dev) */ iommu_alloc_default_domain(group, dev); - if (group->default_domain) + if (group->default_domain) { ret = __iommu_attach_device(group->default_domain, dev); + if (ret) { + iommu_group_put(group); + goto err_release; + } + } iommu_create_device_direct_mappings(group, dev); iommu_group_put(group); - if (ret) - goto err_release; - if (ops->probe_finalize) ops->probe_finalize(dev);
Currently iommu_create_device_direct_mappings() is called without checking the return of __iommu_attach_device(). This may result in failures in iommu driver if dev attach returns error. Fixes: ce574c27ae27("iommu: Move iommu_group_create_direct_mappings() out of iommu_group_add_device()") Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> --- Crash log: [ 31.353605] hns3 0000:7d:00.3: Adding to iommu group 10 [ 31.358822] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000018 [ 31.367567] Mem abort info: [ 31.370350] ESR = 0x96000004 [ 31.373391] EC = 0x25: DABT (current EL), IL = 32 bits [ 31.378680] SET = 0, FnV = 0 [ 31.381720] EA = 0, S1PTW = 0 [ 31.384847] Data abort info: [ 31.387716] ISV = 0, ISS = 0x00000004 [ 31.391535] CM = 0, WnR = 0 [ 31.394491] [0000000000000018] user address but active_mm is swapper [ 31.400818] Internal error: Oops: 96000004 [#1] PREEMPT SMP [ 31.406365] Modules linked in: [ 31.409409] CPU: 21 PID: 1 Comm: swapper/0 Not tainted 5.10.0-rc4-00008-gdd5aba9d719-dirty #79 [ 31.417980] Hardware name: Huawei TaiShan 200 (Model 2280)/BC82AMDD, BIOS 2280-V2 CS V3.B220.01 03/19/2020 [ 31.427588] pstate: 00c00009 (nzcv daif +PAN +UAO -TCO BTYPE=--) [ 31.433566] pc : arm_smmu_tlb_inv_range+0x178/0x1f0 [ 31.438422] lr : arm_smmu_tlb_inv_range+0x5c/0x1f0 [ 31.443190] sp : ffff80001043b4e0 ... [ 31.531175] Call trace: [ 31.533613] arm_smmu_tlb_inv_range+0x178/0x1f0 [ 31.538122] arm_smmu_iotlb_sync+0x2c/0x38 [ 31.542200] iommu_unmap+0x60/0x90 [ 31.545585] __iommu_map+0x110/0x1f0 [ 31.549144] iommu_create_device_direct_mappings.isra.34+0x1ac/0x250 [ 31.555468] iommu_probe_device+0x6c/0x110 [ 31.559551] iort_iommu_configure_id+0x114/0x218 [ 31.564148] acpi_dma_configure_id+0x94/0xe0 [ 31.568402] pci_dma_configure+0xc8/0xf0 [ 31.572310] really_probe+0xd4/0x3e0 [ 31.575871] driver_probe_device+0x5c/0xc0 --- drivers/iommu/iommu.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)