[RFC] driver core: Stop driver bind on NOTIFY_BAD

Commit Message

Alex Williamson June 26, 2017, 7:35 p.m. UTC

Allow participants in the BUS_NOTIFY_BIND_DRIVER to prevent driver
binding with a NOTIFY_BAD.  An example case where this might be useful
is when we're dealing with IOMMU groups and userspace drivers.  We've
defined that devices within the same IOMMU group are not necessarily
DMA isolated from one another and therefore allowing those devices to
be split between host and user drivers may compromise the kernel.  The
vfio driver currently handles this with a BUG_ON when such a condition
occurs.  A better solution is to prevent the case from occurring,
which this change enables.

Signed-off-by: Alex Williamson <[e-mail]>
Suggested-by: Russell King <[e-mail]>
---
 drivers/base/dd.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

For due diligence, none of the current notifier blocks registered with
bus_register_notifier() return anything other than { 0, NOTIFY_OK,
NOTIFY_DONE } except for the case of BUS_NOTIFY_ADD_DEVICE, where
NOTIFY_BAD is possible for NULL data in keystone_platform_notifier()
and an errno return is possible from tce_iommu_bus_notifier() and
i2cdev_notifier_call().  device_add() also ignores the call chain
return value, so these three cases are all ineffective at preventing
anything.

If this is acceptable, I'll re-spin https://lkml.org/lkml/2017/6/20/681
dropping the last 3 patches, instead using the patch below, plumbing
the IOMMU group notifier to percolate notifier block returns, and
simply return NOTIFY_BAD from vfio rather than mucking with
driver_override.  Thanks

Alex

Comments

Greg Kroah-Hartman June 27, 2017, 7 a.m. UTC | #1

On Mon, Jun 26, 2017 at 01:35:50PM -0600, Alex Williamson wrote:
> Allow participants in the BUS_NOTIFY_BIND_DRIVER to prevent driver
> binding with a NOTIFY_BAD.  An example case where this might be useful
> is when we're dealing with IOMMU groups and userspace drivers.  We've
> defined that devices within the same IOMMU group are not necessarily
> DMA isolated from one another and therefore allowing those devices to
> be split between host and user drivers may compromise the kernel.  The
> vfio driver currently handles this with a BUG_ON when such a condition
> occurs.  A better solution is to prevent the case from occurring,
> which this change enables.
> 
> Signed-off-by: Alex Williamson <[e-mail]>
> Suggested-by: Russell King <[e-mail]>
> ---
>  drivers/base/dd.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> For due diligence, none of the current notifier blocks registered with
> bus_register_notifier() return anything other than { 0, NOTIFY_OK,
> NOTIFY_DONE } except for the case of BUS_NOTIFY_ADD_DEVICE, where
> NOTIFY_BAD is possible for NULL data in keystone_platform_notifier()
> and an errno return is possible from tce_iommu_bus_notifier() and
> i2cdev_notifier_call().  device_add() also ignores the call chain
> return value, so these three cases are all ineffective at preventing
> anything.
> 
> If this is acceptable, I'll re-spin https://lkml.org/lkml/2017/6/20/681
> dropping the last 3 patches, instead using the patch below, plumbing
> the IOMMU group notifier to percolate notifier block returns, and
> simply return NOTIFY_BAD from vfio rather than mucking with
> driver_override.  Thanks
> 
> Alex 
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 4882f06d12df..32c1d841e8d9 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -265,9 +265,11 @@ static int driver_sysfs_add(struct device *dev)
>  {
>  	int ret;
>  
> -	if (dev->bus)
> -		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> -					     BUS_NOTIFY_BIND_DRIVER, dev);
> +	if (dev->bus) {
> +		if (blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> +				BUS_NOTIFY_BIND_DRIVER, dev) == NOTIFY_BAD)
> +			return -EINVAL;
> +	}

Ick, this seems really hacky.  Right now we do not do anything on any of
the bus notifiers, so why start doing it now?

How is this ever being called anyway?  Your driver should have rejected
being bound to the device at all, much earlier in your probe function.

Or are you somehow trying to keep userspace from manually binding the
driver to the device?  If so, why not just disable that functionality
for it (there is a bit somewhere for that...)

thanks,

greg k-h

Alex Williamson June 27, 2017, 8:18 p.m. UTC | #2

On Tue, 27 Jun 2017 09:00:45 +0200
Greg KH <[e-mail]> wrote:

> On Mon, Jun 26, 2017 at 01:35:50PM -0600, Alex Williamson wrote:
> > Allow participants in the BUS_NOTIFY_BIND_DRIVER to prevent driver
> > binding with a NOTIFY_BAD.  An example case where this might be useful
> > is when we're dealing with IOMMU groups and userspace drivers.  We've
> > defined that devices within the same IOMMU group are not necessarily
> > DMA isolated from one another and therefore allowing those devices to
> > be split between host and user drivers may compromise the kernel.  The
> > vfio driver currently handles this with a BUG_ON when such a condition
> > occurs.  A better solution is to prevent the case from occurring,
> > which this change enables.
> > 
> > Signed-off-by: Alex Williamson <[e-mail]>
> > Suggested-by: Russell King <[e-mail]>
> > ---
> >  drivers/base/dd.c |    8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > For due diligence, none of the current notifier blocks registered with
> > bus_register_notifier() return anything other than { 0, NOTIFY_OK,
> > NOTIFY_DONE } except for the case of BUS_NOTIFY_ADD_DEVICE, where
> > NOTIFY_BAD is possible for NULL data in keystone_platform_notifier()
> > and an errno return is possible from tce_iommu_bus_notifier() and
> > i2cdev_notifier_call().  device_add() also ignores the call chain
> > return value, so these three cases are all ineffective at preventing
> > anything.
> > 
> > If this is acceptable, I'll re-spin https://lkml.org/lkml/2017/6/20/681
> > dropping the last 3 patches, instead using the patch below, plumbing
> > the IOMMU group notifier to percolate notifier block returns, and
> > simply return NOTIFY_BAD from vfio rather than mucking with
> > driver_override.  Thanks
> > 
> > Alex 
> > 
> > diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> > index 4882f06d12df..32c1d841e8d9 100644
> > --- a/drivers/base/dd.c
> > +++ b/drivers/base/dd.c
> > @@ -265,9 +265,11 @@ static int driver_sysfs_add(struct device *dev)
> >  {
> >  	int ret;
> >  
> > -	if (dev->bus)
> > -		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> > -					     BUS_NOTIFY_BIND_DRIVER, dev);
> > +	if (dev->bus) {
> > +		if (blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
> > +				BUS_NOTIFY_BIND_DRIVER, dev) == NOTIFY_BAD)
> > +			return -EINVAL;
> > +	}  
> 
> Ick, this seems really hacky.  Right now we do not do anything on any of
> the bus notifiers, so why start doing it now?
> 
> How is this ever being called anyway?  Your driver should have rejected
> being bound to the device at all, much earlier in your probe function.
> 
> Or are you somehow trying to keep userspace from manually binding the
> driver to the device?  If so, why not just disable that functionality
> for it (there is a bit somewhere for that...)

An example scenario is that we have an IOMMU group with multiple
devices.  We assume that devices within the same IOMMU group are not DMA
isolated from one another and therefore having host-owned and user-owned
devices within the same IOMMU group compromises the host integrity.
Therefore if a device within a group is in use by the user while
another device within the same group is unbound from its vfio driver
and bound to a host driver, what do we do?

The driver model doesn't support us returning an error from an unbind
request, the best we can do would be to block the unbind from the vfio
driver, but that has its own set of issues.  So our hand is forced to
allow the unbind, but then what?

Currently if the device is then bound to a host driver, vfio will
trigger a BUG_ON since we consider the system integrity compromised.
That's not good.  Unless we add some sort of IOMMU group smarts to the
other driver or the driver core, nobody else has visibility to this
issue, ie. the binding driver is any another driver, it's just a
bystander to this situation.  Disabling autoprobe doesn't really solve
anything, tools like libvirt can still put us in this situation
regardless of autoprobe and even if we think an admin user should be
able to shoot themselves in the foot, perhaps we shouldn't make it so
easy and non-obvious for them to do so.

The proposal here is that if we provide a mechanism for a participant
in the bus notifier chain to nak a driver bind, then we can prevent the
compromising scenario, which seems like an improvement from the current
response.  As I note above, we do already have instances where
participants in the notifier chain think they do have voting status and
I don't really see an issue with making that be true in cases where it
can improve the system behavior.  Thanks,

Alex

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 4882f06d12df..32c1d841e8d9 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -265,9 +265,11 @@  static int driver_sysfs_add(struct device *dev)
 {
 	int ret;
 
-	if (dev->bus)
-		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
-					     BUS_NOTIFY_BIND_DRIVER, dev);
+	if (dev->bus) {
+		if (blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
+				BUS_NOTIFY_BIND_DRIVER, dev) == NOTIFY_BAD)
+			return -EINVAL;
+	}
 
 	ret = sysfs_create_link(&dev->driver->p->kobj, &dev->kobj,
 			  kobject_name(&dev->kobj));