Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1713812 > unrolled thread

[PATCH] iommu: Avoid NULL group dereference

Back to article view | Back to linux.kernel

Contents

  [PATCH] iommu: Avoid NULL group dereference Robin Murphy <robin.murphy@arm.com> - 2017-08-17 12:50 +0200
    Re: [PATCH] iommu: Avoid NULL group dereference Marc Zyngier <marc.zyngier@arm.com> - 2017-08-17 13:00 +0200
    Re: [PATCH] iommu: Avoid NULL group dereference Joerg Roedel <joro@8bytes.org> - 2017-08-17 17:50 +0200
      Re: [PATCH] iommu: Avoid NULL group dereference Robin Murphy <robin.murphy@arm.com> - 2017-08-17 19:00 +0200
        Re: [PATCH] iommu: Avoid NULL group dereference Joerg Roedel <joro@8bytes.org> - 2017-08-18 11:50 +0200
    Re: [PATCH] iommu: Avoid NULL group dereference Shawn Lin <shawn.lin@rock-chips.com> - 2017-08-18 03:00 +0200

#1713812 — [PATCH] iommu: Avoid NULL group dereference

The recently-removed FIXME in iommu_get_domain_for_dev() turns out to
have been a little misleading, since that check is still worthwhile even
when groups *are* universal. We have a few IOMMU-aware drivers which
only care whether their device is already attached to an existing domain
or not, for which the previous behaviour of iommu_get_domain_for_dev()
was ideal, and who now crash if their device does not have an IOMMU.

With IOMMU groups now serving as a reliable indicator of whether a
device has an IOMMU or not (barring false-positives from VFIO no-IOMMU
mode), drivers could arguably do this:

	group = iommu_group_get(dev);
	if (group) {
		domain = iommu_get_domain_for_dev(dev);
		iommu_group_put(group);
	}

However, rather than duplicate that code across multiple callsites,
particularly when it's still only the domain they care about, let's skip
straight to the next step and factor out the check into the common place
it applies - in iommu_get_domain_for_dev() itself. Sure, it ends up
looking rather familiar, but now it's backed by the reasoning of having
a robust API able to do the expected thing for all devices regardless.

Fixes: 05f80300dc8b ("iommu: Finish making iommu_group support mandatory")
Reported-by: Shawn Lin <shawn.lin@rock-chips.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

As well as dma-iommu, there are at least the Cavium ThunderX and
Freescale DPAA2 ethernet drivers expecting this to work too.

 drivers/iommu/iommu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index af69bf7e035a..5499a0387349 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1352,6 +1352,8 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
 	struct iommu_group *group;
 
 	group = iommu_group_get(dev);
+	if (!group)
+		return NULL;
 
 	domain = group->domain;
 
-- 
2.13.4.dirty

[toc] | [next] | [standalone]

#1713817

On 17/08/17 11:40, Robin Murphy wrote:
> The recently-removed FIXME in iommu_get_domain_for_dev() turns out to
> have been a little misleading, since that check is still worthwhile even
> when groups *are* universal. We have a few IOMMU-aware drivers which
> only care whether their device is already attached to an existing domain
> or not, for which the previous behaviour of iommu_get_domain_for_dev()
> was ideal, and who now crash if their device does not have an IOMMU.
> 
> With IOMMU groups now serving as a reliable indicator of whether a
> device has an IOMMU or not (barring false-positives from VFIO no-IOMMU
> mode), drivers could arguably do this:
> 
> 	group = iommu_group_get(dev);
> 	if (group) {
> 		domain = iommu_get_domain_for_dev(dev);
> 		iommu_group_put(group);
> 	}
> 
> However, rather than duplicate that code across multiple callsites,
> particularly when it's still only the domain they care about, let's skip
> straight to the next step and factor out the check into the common place
> it applies - in iommu_get_domain_for_dev() itself. Sure, it ends up
> looking rather familiar, but now it's backed by the reasoning of having
> a robust API able to do the expected thing for all devices regardless.
> 
> Fixes: 05f80300dc8b ("iommu: Finish making iommu_group support mandatory")
> Reported-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1714165

On Thu, Aug 17, 2017 at 11:40:08AM +0100, Robin Murphy wrote:
> The recently-removed FIXME in iommu_get_domain_for_dev() turns out to
> have been a little misleading, since that check is still worthwhile even
> when groups *are* universal. We have a few IOMMU-aware drivers which
> only care whether their device is already attached to an existing domain
> or not, for which the previous behaviour of iommu_get_domain_for_dev()
> was ideal, and who now crash if their device does not have an IOMMU.
> 
> With IOMMU groups now serving as a reliable indicator of whether a
> device has an IOMMU or not (barring false-positives from VFIO no-IOMMU
> mode), drivers could arguably do this:
> 
> 	group = iommu_group_get(dev);
> 	if (group) {
> 		domain = iommu_get_domain_for_dev(dev);
> 		iommu_group_put(group);
> 	}

Okay, so just to check I got it right: Drivers do the above to check
whether a device is managed by an IOMMU, and that crashes now because
the 'group == NULL' check was removed?

Regards,

	Joerg

[toc] | [prev] | [next] | [standalone]

#1714205

On 17/08/17 16:41, Joerg Roedel wrote:
> On Thu, Aug 17, 2017 at 11:40:08AM +0100, Robin Murphy wrote:
>> The recently-removed FIXME in iommu_get_domain_for_dev() turns out to
>> have been a little misleading, since that check is still worthwhile even
>> when groups *are* universal. We have a few IOMMU-aware drivers which
>> only care whether their device is already attached to an existing domain
>> or not, for which the previous behaviour of iommu_get_domain_for_dev()
>> was ideal, and who now crash if their device does not have an IOMMU.
>>
>> With IOMMU groups now serving as a reliable indicator of whether a
>> device has an IOMMU or not (barring false-positives from VFIO no-IOMMU
>> mode), drivers could arguably do this:
>>
>> 	group = iommu_group_get(dev);
>> 	if (group) {
>> 		domain = iommu_get_domain_for_dev(dev);
>> 		iommu_group_put(group);
>> 	}
> 
> Okay, so just to check I got it right: Drivers do the above to check
> whether a device is managed by an IOMMU, and that crashes now because
> the 'group == NULL' check was removed?

Indeed - the typical context is network descriptors that don't have
space to store the CPU virtual address of the buffer, so when a packet
arrives the driver has to work backwards from the DMA address, in this
sort of pattern:

	addr = desc[idx]->addr;
	domain = iommu_get_domain_for_dev(dev);
	if (domain)
		addr = iommu_iova_to_phys(domain, addr)
	buf = phys_to_virt(addr)

(the GIC thing is similar but in reverse, with a physical address which
may or may not need replacing with an IOVA). Unless we were to change
the interface to be iommu_get_domain_for_group(), I think it makes sense
for it to remain valid to call for any device.

Robin.

[toc] | [prev] | [next] | [standalone]

#1714766

On Thu, Aug 17, 2017 at 05:56:23PM +0100, Robin Murphy wrote:
> On 17/08/17 16:41, Joerg Roedel wrote:
> > Okay, so just to check I got it right: Drivers do the above to check
> > whether a device is managed by an IOMMU, and that crashes now because
> > the 'group == NULL' check was removed?
> 
> Indeed - the typical context is network descriptors that don't have
> space to store the CPU virtual address of the buffer, so when a packet
> arrives the driver has to work backwards from the DMA address, in this
> sort of pattern:
> 
> 	addr = desc[idx]->addr;
> 	domain = iommu_get_domain_for_dev(dev);
> 	if (domain)
> 		addr = iommu_iova_to_phys(domain, addr)
> 	buf = phys_to_virt(addr)
> 
> (the GIC thing is similar but in reverse, with a physical address which
> may or may not need replacing with an IOVA). Unless we were to change
> the interface to be iommu_get_domain_for_group(), I think it makes sense
> for it to remain valid to call for any device.

Okay, we should find a better solution for this at some point, but I
applied the patch for now.


	Joerg

[toc] | [prev] | [next] | [standalone]

#1714497

Hi

On 2017/8/17 18:40, Robin Murphy wrote:
> The recently-removed FIXME in iommu_get_domain_for_dev() turns out to
> have been a little misleading, since that check is still worthwhile even
> when groups *are* universal. We have a few IOMMU-aware drivers which
> only care whether their device is already attached to an existing domain
> or not, for which the previous behaviour of iommu_get_domain_for_dev()
> was ideal, and who now crash if their device does not have an IOMMU.
> 

It works, thanks!

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>


> With IOMMU groups now serving as a reliable indicator of whether a
> device has an IOMMU or not (barring false-positives from VFIO no-IOMMU
> mode), drivers could arguably do this:
> 
> 	group = iommu_group_get(dev);
> 	if (group) {
> 		domain = iommu_get_domain_for_dev(dev);
> 		iommu_group_put(group);
> 	}
> 
> However, rather than duplicate that code across multiple callsites,
> particularly when it's still only the domain they care about, let's skip
> straight to the next step and factor out the check into the common place
> it applies - in iommu_get_domain_for_dev() itself. Sure, it ends up
> looking rather familiar, but now it's backed by the reasoning of having
> a robust API able to do the expected thing for all devices regardless.
> 
> Fixes: 05f80300dc8b ("iommu: Finish making iommu_group support mandatory")
> Reported-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> As well as dma-iommu, there are at least the Cavium ThunderX and
> Freescale DPAA2 ethernet drivers expecting this to work too.
> 
>   drivers/iommu/iommu.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index af69bf7e035a..5499a0387349 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1352,6 +1352,8 @@ struct iommu_domain *iommu_get_domain_for_dev(struct device *dev)
>   	struct iommu_group *group;
>   
>   	group = iommu_group_get(dev);
> +	if (!group)
> +		return NULL;
>   
>   	domain = group->domain;
>   
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web