| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 10 Oct 2013 08:01:22 +0000
From: Bhushan Bharat-R65777 <R65777@...escale.com>
To: Kim Phillips <kim.phillips@...aro.org>,
Wood Scott-B07421 <B07421@...escale.com>
CC: Yoder Stuart-B08248 <B08248@...escale.com>,
Wood Scott-B07421 <B07421@...escale.com>,
"christoffer.dall@...aro.org" <christoffer.dall@...aro.org>,
"alex.williamson@...hat.com" <alex.williamson@...hat.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"a.motakis@...tualopensystems.com" <a.motakis@...tualopensystems.com>,
"agraf@...e.de" <agraf@...e.de>,
Sethi Varun-B16395 <B16395@...escale.com>,
"peter.maydell@...aro.org" <peter.maydell@...aro.org>,
"santosh.shukla@...aro.org" <santosh.shukla@...aro.org>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: RE: RFC: (re-)binding the VFIO platform driver to a platform device
> -----Original Message-----
> From: kvm-owner@...r.kernel.org [mailto:kvm-owner@...r.kernel.org] On Behalf Of
> Kim Phillips
> Sent: Thursday, October 10, 2013 8:36 AM
> To: Wood Scott-B07421
> Cc: Yoder Stuart-B08248; Wood Scott-B07421; christoffer.dall@...aro.org;
> alex.williamson@...hat.com; linux-kernel@...r.kernel.org;
> a.motakis@...tualopensystems.com; agraf@...e.de; Sethi Varun-B16395; Bhushan
> Bharat-R65777; peter.maydell@...aro.org; santosh.shukla@...aro.org;
> kvm@...r.kernel.org; gregkh@...uxfoundation.org
> Subject: Re: RFC: (re-)binding the VFIO platform driver to a platform device
>
> On Wed, 9 Oct 2013 15:03:19 -0500
> Scott Wood <scottwood@...escale.com> wrote:
>
> > On Wed, 2013-10-09 at 14:44 -0500, Yoder Stuart-B08248 wrote:
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, October 09, 2013 2:22 PM
> > > >
> > > > On Wed, 2013-10-09 at 14:02 -0500, Yoder Stuart-B08248 wrote:
> > > > > Have been thinking about this issue some more. As Scott
> > > > > mentioned,
>
> thanks for bringing this up again.
>
> > > > There's already a "bool suppress_bind_attrs" to prevent sysfs
> > > > bind/unbind. I suggested a similar flag to mean the oppsosite --
> > > > bind
> > > > *only* through sysfs. Greg KH was skeptical and wanted to see a
> > > > patch before any further discussion.
> > >
> > > Ah, think I understand now...yes that works as well, and would be
> > > less intrustive. So are you writing a patch? :)
> >
> > I've been meaning to since the previous round of discussion, but I've
> > been busy. Would someone else be able to test it in the context of
> > using it for VFIO?
>
> yes - see below.
>
> > Otherwise, that looks about right, for the driver side (though
> > driver_attach could error out earlier rather than testing it inside
> > the loop).
>
> I've made the changes you suggested and tested the resulting diff below on an
> arndale board. I successfully performed the following sequence of commands
> after first changing the i2c@...80000 node in the device tree to be exclusively
> compatible with "vfio":
>
> ===
> # ls -l /sys/bus/platform/drivers/vfio-platform/
> total 0
> --w------- 1 root root 4096 Sep 24 19:17 bind
> --w------- 1 root root 4096 Sep 24 19:13 uevent
> --w------- 1 root root 4096 Sep 24 19:18 unbind # ls -l
> /sys/bus/platform/drivers/s3c-i2c total 0
> lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c60000.i2c ->
> ../../../../devices/12c60000.i2c
> lrwxrwxrwx 1 root root 0 Sep 24 19:11 12c90000.i2c ->
> ../../../../devices/12c90000.i2c
> lrwxrwxrwx 1 root root 0 Sep 24 19:20 12ce0000.i2c ->
> ../../../../devices/12ce0000.i2c
> --w------- 1 root root 4096 Sep 24 19:18 bind
> --w------- 1 root root 4096 Sep 24 19:11 uevent
> --w------- 1 root root 4096 Sep 24 19:17 unbind # ls -l
> /sys/devices/12c80000.i2c/driver # this is the one with the 'vfio' compatible
> ls: cannot access /sys/devices/12c80000.i2c/driver: No such file or directory #
> ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:18
> /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/s3c-i2c
> # echo 12ce0000.i2c > /sys/bus/platform/drivers/s3c-i2c/unbind
> # ls -l /sys/devices/12ce0000.i2c/driver
> ls: cannot access /sys/devices/12ce0000.i2c/driver: No such file or directory #
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
> # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1 root root 0 Sep 24 19:21
> /sys/devices/12ce0000.i2c/driver -> ../../bus/platform/drivers/vfio-platform
> # echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/unbind
> # ls -l /sys/devices/12ce0000.i2c/driver # echo 12ce0000.i2c >
> /sys/bus/platform/drivers/s3c-i2c/bind
> [ 722.137524] s3c-i2c 12ce0000.i2c: slave address 0x38 [ 722.141037] s3c-i2c
> 12ce0000.i2c: bus frequency set to 65 KHz [ 722.150605] s3c-i2c 12ce0000.i2c:
> i2c-8: S3C I2C adapter # ls -l /sys/devices/12ce0000.i2c/driver lrwxrwxrwx 1
> root root 0 Sep 24 19:21 /sys/devices/12ce0000.i2c/driver ->
> ../../bus/platform/drivers/s3c-i2c
> #
> ====
>
> so it's correctly not allowing 'vfio' driver to bind to a device tree compatible
> it's declared, and it then can bind the i2c @ 12ce0000 device to the vfio-
> platform driver, and unbind and bind it back to the i2c driver.
>
> For clarity's sake, before this diff, the command:
>
> echo 12ce0000.i2c > /sys/bus/platform/drivers/vfio-platform/bind
>
> would error with:
>
> echo: write error: No such device
>
> > The other half of fixing the raciness is to ensure that the device
> > doesn't get bound back to a non-VFIO driver (e.g. due to a module load
> > or new_id). The solution I proposed for that was a similar
> > explicit-bind-only flag for a device, that the user sets through sysfs
> > prior to unbinding. This would also be useful in non-VFIO contexts to
> > simply say "I don't want to use this device at all".
>
> I can take a look at doing this if you're still busy.
>
> Thanks,
>
> Kim
>
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 73f6c29..da81442
> 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -201,7 +201,8 @@ static ssize_t bind_store(struct device_driver *drv, const
> char *buf,
> int err = -ENODEV;
>
> dev = bus_find_device_by_name(bus, NULL, buf);
> - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) {
> + if (dev && dev->driver == NULL && (drv->sysfs_bind_only ||
> + driver_match_device(drv, dev))) {
Should not we check
if (dev && dev->driver == NULL &&
(device->explicit_bind_only && drv->explicit_bind_only) ||
driver_match_device(drv, dev)))
> if (dev->parent) /* Needed for USB */
> device_lock(dev->parent);
> device_lock(dev);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 35fa368..6f85279 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -389,7 +389,7 @@ static int __device_attach(struct device_driver *drv, void
> *data) {
> struct device *dev = data;
>
> - if (!driver_match_device(drv, dev))
> + if (drv->sysfs_bind_only || !driver_match_device(drv, dev))
Likewise ..
Thanks
-Bharat
> return 0;
>
> return driver_probe_device(drv, dev);
> @@ -476,6 +476,9 @@ static int __driver_attach(struct device *dev, void *data)
> */
> int driver_attach(struct device_driver *drv) {
> + if (drv->sysfs_bind_only)
> + return 0;
> +
> return bus_for_each_dev(drv->bus, NULL, drv, __driver_attach); }
> EXPORT_SYMBOL_GPL(driver_attach); diff --git a/drivers/vfio/vfio_platform.c
> b/drivers/vfio/vfio_platform.c index b92d7bb..ba578b2 100644
> --- a/drivers/vfio/vfio_platform.c
> +++ b/drivers/vfio/vfio_platform.c
> @@ -297,6 +297,7 @@ static struct platform_driver vfio_platform_driver = {
> .name = "vfio-platform",
> .owner = THIS_MODULE,
> .of_match_table = vfio_platform_match,
> + .sysfs_bind_only = true,
> },
> };
>
> diff --git a/include/linux/device.h b/include/linux/device.h index
> 94638ef..a3ae81e 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -199,6 +199,7 @@ extern struct klist *bus_get_device_klist(struct bus_type
> *bus);
> * @owner: The module owner.
> * @mod_name: Used for built-in modules.
> * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @sysfs_bind_only: Only allow bind/unbind via sysfs.
> * @of_match_table: The open firmware table.
> * @acpi_match_table: The ACPI match table.
> * @probe: Called to query the existence of a specific device,
> @@ -232,6 +233,7 @@ struct device_driver {
> const char *mod_name; /* used for built-in modules */
>
> bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
> + bool sysfs_bind_only; /* only allow bind/unbind via sysfs */
>
> const struct of_device_id *of_match_table;
> const struct acpi_device_id *acpi_match_table;
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a
> message to majordomo@...r.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists