Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Alan Stern]

On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> version of device authorization to selectively authorize the driver
> probes. Since there is a common requirement, move the "authorized"
> attribute support to the driver core in order to allow it to be used
> by other subsystems / buses.
> 
> Similar requirements have been discussed in the PCI [1] community for
> PCI bus drivers as well.
> 
> No functional changes are intended. It just converts authorized
> attribute from int to bool and moves it to the driver core. There
> should be no user-visible change in the location or semantics of
> attributes for USB devices.
> 
> Regarding thunderbolt driver, although it declares sw->authorized as
> "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> but within the driver, in all authorized attribute related checks,
> it is treated as bool value. So when converting the authorized
> attribute from int to bool value, there should be no functional
> changes other than value 2 being not visible to the user.
> 
> [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Since you're moving the authorized flag from the USB core to the
driver core, the corresponding sysfs attribute functions should be
moved as well.

Also, you ignored the usb_[de]authorize_interface() functions and 
their friends.

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Dan Williams]

On Wed, Sep 29, 2021 at 6:43 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> > version of device authorization to selectively authorize the driver
> > probes. Since there is a common requirement, move the "authorized"
> > attribute support to the driver core in order to allow it to be used
> > by other subsystems / buses.
> >
> > Similar requirements have been discussed in the PCI [1] community for
> > PCI bus drivers as well.
> >
> > No functional changes are intended. It just converts authorized
> > attribute from int to bool and moves it to the driver core. There
> > should be no user-visible change in the location or semantics of
> > attributes for USB devices.
> >
> > Regarding thunderbolt driver, although it declares sw->authorized as
> > "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> > but within the driver, in all authorized attribute related checks,
> > it is treated as bool value. So when converting the authorized
> > attribute from int to bool value, there should be no functional
> > changes other than value 2 being not visible to the user.
> >
> > [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> >
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Since you're moving the authorized flag from the USB core to the
> driver core, the corresponding sysfs attribute functions should be
> moved as well.

Unlike when 'removable' moved from USB to the driver core there isn't
a common definition for how the 'authorized' sysfs-attribute behaves
across buses. The only common piece is where this flag is stored in
the data structure, i.e. the 'authorized' sysfs interface is
purposefully left bus specific.

> Also, you ignored the usb_[de]authorize_interface() functions and
> their friends.

Ugh, yes.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Dan Williams]

On Wed, Sep 29, 2021 at 7:39 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>
> On 9/29/21 6:55 PM, Dan Williams wrote:
> >> Also, you ignored the usb_[de]authorize_interface() functions and
> >> their friends.
> > Ugh, yes.
>
> I did not change it because I am not sure about the interface vs device
> dependency.
>

This is was the rationale for has_probe_authorization flag. USB
performs authorization of child devices based on the authorization
state of the parent interface.

> I think following change should work.
>
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index f57b5a7a90ca..84969732d09c 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -334,7 +334,7 @@ static int usb_probe_interface(struct device *dev)
>         if (udev->dev.authorized == false) {
>                 dev_err(&intf->dev, "Device is not authorized for usage\n");
>                 return error;
> -       } else if (intf->authorized == 0) {
> +       } else if (intf->dev.authorized == 0) {

== false.

>                 dev_err(&intf->dev, "Interface %d is not authorized for usage\n",
>                                 intf->altsetting->desc.bInterfaceNumber);
>                 return error;
> @@ -546,7 +546,7 @@ int usb_driver_claim_interface(struct usb_driver *driver,
>                 return -EBUSY;
>
>         /* reject claim if interface is not authorized */
> -       if (!iface->authorized)
> +       if (!iface->dev.authorized)

I'd do == false to keep it consistent with other conversions.

>                 return -ENODEV;
>
>         dev->driver = &driver->drvwrap.driver;
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 47548ce1cfb1..ab3c8d1e4db9 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1791,9 +1791,9 @@ void usb_deauthorize_interface(struct usb_interface *intf)
>
>         device_lock(dev->parent);
>
> -       if (intf->authorized) {
> +       if (intf->dev.authorized) {
>                 device_lock(dev);
> -               intf->authorized = 0;
> +               intf->dev.authorized = 0;

= false;

>                 device_unlock(dev);
>
>                 usb_forced_unbind_intf(intf);
> @@ -1811,9 +1811,9 @@ void usb_authorize_interface(struct usb_interface *intf)
>   {
>         struct device *dev = &intf->dev;
>
> -       if (!intf->authorized) {
> +       if (!intf->dev.authorized) {
>                 device_lock(dev);
> -               intf->authorized = 1; /* authorize interface */
> +               intf->dev.authorized = 1; /* authorize interface */

= true

...not sure that comment is worth preserving.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Rafael J. Wysocki]

On Thu, Sep 30, 2021 at 6:59 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Wed, Sep 29, 2021 at 7:39 PM Kuppuswamy, Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >
> >
> >
> > On 9/29/21 6:55 PM, Dan Williams wrote:
> > >> Also, you ignored the usb_[de]authorize_interface() functions and
> > >> their friends.
> > > Ugh, yes.
> >
> > I did not change it because I am not sure about the interface vs device
> > dependency.
> >
>
> This is was the rationale for has_probe_authorization flag. USB
> performs authorization of child devices based on the authorization
> state of the parent interface.
>
> > I think following change should work.
> >
> > diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> > index f57b5a7a90ca..84969732d09c 100644
> > --- a/drivers/usb/core/driver.c
> > +++ b/drivers/usb/core/driver.c
> > @@ -334,7 +334,7 @@ static int usb_probe_interface(struct device *dev)
> >         if (udev->dev.authorized == false) {
> >                 dev_err(&intf->dev, "Device is not authorized for usage\n");
> >                 return error;
> > -       } else if (intf->authorized == 0) {
> > +       } else if (intf->dev.authorized == 0) {
>
> == false

Or even (!intf->dev.authorized).

> >                 dev_err(&intf->dev, "Interface %d is not authorized for usage\n",
> >                                 intf->altsetting->desc.bInterfaceNumber);
> >                 return error;
> > @@ -546,7 +546,7 @@ int usb_driver_claim_interface(struct usb_driver *driver,
> >                 return -EBUSY;
> >
> >         /* reject claim if interface is not authorized */
> > -       if (!iface->authorized)
> > +       if (!iface->dev.authorized)
>
> I'd do == false to keep it consistent with other conversions.

But this looks odd, FWIW.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Alan Stern]

On Wed, Sep 29, 2021 at 06:55:12PM -0700, Dan Williams wrote:
> On Wed, Sep 29, 2021 at 6:43 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> > > version of device authorization to selectively authorize the driver
> > > probes. Since there is a common requirement, move the "authorized"
> > > attribute support to the driver core in order to allow it to be used
> > > by other subsystems / buses.
> > >
> > > Similar requirements have been discussed in the PCI [1] community for
> > > PCI bus drivers as well.
> > >
> > > No functional changes are intended. It just converts authorized
> > > attribute from int to bool and moves it to the driver core. There
> > > should be no user-visible change in the location or semantics of
> > > attributes for USB devices.
> > >
> > > Regarding thunderbolt driver, although it declares sw->authorized as
> > > "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> > > but within the driver, in all authorized attribute related checks,
> > > it is treated as bool value. So when converting the authorized
> > > attribute from int to bool value, there should be no functional
> > > changes other than value 2 being not visible to the user.
> > >
> > > [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > >
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >
> > Since you're moving the authorized flag from the USB core to the
> > driver core, the corresponding sysfs attribute functions should be
> > moved as well.
> 
> Unlike when 'removable' moved from USB to the driver core there isn't
> a common definition for how the 'authorized' sysfs-attribute behaves
> across buses. The only common piece is where this flag is stored in
> the data structure, i.e. the 'authorized' sysfs interface is
> purposefully left bus specific.

How about implementing "library" versions of show_authorized() and 
store_authorized() that the bus-specific attribute routines can call?  
These library routines would handle parsing the input values, storing 
the new flag, and displaying the stored flag value.  That way at 
least the common parts of these APIs would be centralized in the 
driver core, and any additional functionality could easily be added 
by the bus-specific attribute routine.

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Dan Williams]

On Thu, Sep 30, 2021 at 8:00 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Sep 29, 2021 at 06:55:12PM -0700, Dan Williams wrote:
> > On Wed, Sep 29, 2021 at 6:43 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, Sep 29, 2021 at 06:05:06PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > Currently bus drivers like "USB" or "Thunderbolt" implement a custom
> > > > version of device authorization to selectively authorize the driver
> > > > probes. Since there is a common requirement, move the "authorized"
> > > > attribute support to the driver core in order to allow it to be used
> > > > by other subsystems / buses.
> > > >
> > > > Similar requirements have been discussed in the PCI [1] community for
> > > > PCI bus drivers as well.
> > > >
> > > > No functional changes are intended. It just converts authorized
> > > > attribute from int to bool and moves it to the driver core. There
> > > > should be no user-visible change in the location or semantics of
> > > > attributes for USB devices.
> > > >
> > > > Regarding thunderbolt driver, although it declares sw->authorized as
> > > > "int" and allows 0,1,2 as valid values for sw->authorized attribute,
> > > > but within the driver, in all authorized attribute related checks,
> > > > it is treated as bool value. So when converting the authorized
> > > > attribute from int to bool value, there should be no functional
> > > > changes other than value 2 being not visible to the user.
> > > >
> > > > [1]: https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > >
> > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > >
> > > Since you're moving the authorized flag from the USB core to the
> > > driver core, the corresponding sysfs attribute functions should be
> > > moved as well.
> >
> > Unlike when 'removable' moved from USB to the driver core there isn't
> > a common definition for how the 'authorized' sysfs-attribute behaves
> > across buses. The only common piece is where this flag is stored in
> > the data structure, i.e. the 'authorized' sysfs interface is
> > purposefully left bus specific.
>
> How about implementing "library" versions of show_authorized() and
> store_authorized() that the bus-specific attribute routines can call?
> These library routines would handle parsing the input values, storing
> the new flag, and displaying the stored flag value.  That way at
> least the common parts of these APIs would be centralized in the
> driver core, and any additional functionality could easily be added
> by the bus-specific attribute routine.
>

While show_authorized() seems like it could be standardized, have a
look at what the different store_authorized() implementations do.
Thunderbolt wants "switch approval" vs "switch challenge" and USB has
a bunch of bus-specific work to do when the authorization state
changes. I don't see much room for a library to help there as more
buses add authorization support. That said I do think it would be
useful to have a common implementation available for generic probe
authorization to toggle the flag if the bus does not have any
authorization work to do, but that seems a follow-on once this core is
accepted.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Dan Williams]

On Thu, Sep 30, 2021 at 4:20 AM Yehezkel Bernat <yehezkelshb@gmail.com> wrote:
>
> On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >
> > no functional
> > changes other than value 2 being not visible to the user.
> >
>
> Are we sure we don't break any user-facing tool with it? Tools might use this to
> "remember" how the device was authorized this time.

That's why it was highlighted in the changelog. Hopefully a
Thunderbolt developer can confirm if it is a non-issue.
Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
answer this question about whether authorized_show and
authorized_store need to be symmetric.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Dan Williams]

On Thu, Sep 30, 2021 at 11:25 AM Yehezkel Bernat <yehezkelshb@gmail.com> wrote:
>
> On Thu, Sep 30, 2021 at 6:28 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Thu, Sep 30, 2021 at 4:20 AM Yehezkel Bernat <yehezkelshb@gmail.com> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan
> > > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> > > >
> > > > no functional
> > > > changes other than value 2 being not visible to the user.
> > > >
> > >
> > > Are we sure we don't break any user-facing tool with it? Tools might use this to
> > > "remember" how the device was authorized this time.
> >
> > That's why it was highlighted in the changelog. Hopefully a
> > Thunderbolt developer can confirm if it is a non-issue.
> > Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
> > answer this question about whether authorized_show and
> > authorized_store need to be symmetric.
>
> Apparently, Bolt does read it [1] and cares about it [2].

Ah, thank you!

Yeah, looks like the conversion to bool was indeed too hopeful.

>
> [1] https://gitlab.freedesktop.org/bolt/bolt/-/blob/130e09d1c7ff02c09e4ad1c9c36e9940b68e58d8/boltd/bolt-sysfs.c#L511
> [2] https://gitlab.freedesktop.org/bolt/bolt/-/blob/130e09d1c7ff02c09e4ad1c9c36e9940b68e58d8/boltd/bolt-device.c#L639
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core

[Dan Williams]

On Thu, Sep 30, 2021 at 12:50 PM Kuppuswamy, Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>
> On 9/30/21 12:04 PM, Dan Williams wrote:
> >>> That's why it was highlighted in the changelog. Hopefully a
> >>> Thunderbolt developer can confirm if it is a non-issue.
> >>> Documentation/ABI/testing/sysfs-bus-thunderbolt does not seem to
> >>> answer this question about whether authorized_show and
> >>> authorized_store need to be symmetric.
> >> Apparently, Bolt does read it [1] and cares about it [2].
> > Ah, thank you!
> >
> > Yeah, looks like the conversion to bool was indeed too hopeful.
> >
>
> IIUC, the end result of value "2" in authorized sysfs is to just
> "authorize" or "de-authorize". In that case, can the user space
> driver adapt to this int->bool change? Just want to know the
> possibility.

ABIs are forever. The kernel has to uphold its contract to bolt that
it will return '2' and not '1' after '2' has been written.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Michael S. Tsirkin]

On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> While the common case for device-authorization is to skip probe of
> unauthorized devices, some buses may still want to emit a message on
> probe failure (Thunderbolt), or base probe failures on the
> authorization status of a related device like a parent (USB). So add
> an option (has_probe_authorization) in struct bus_type for the bus
> driver to own probe authorization policy.
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>



So what e.g. the PCI patch
https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
actually proposes is a list of
allowed drivers, not devices. Doing it at the device level
has disadvantages, for example some devices might have a legacy
unsafe driver, or an out of tree driver. It also does not
address drivers that poke at hardware during init.

Accordingly, I think the right thing to do is to skip
driver init for disallowed drivers, not skip probe
for specific devices.


> ---
>  drivers/base/dd.c            | 5 +++++
>  drivers/thunderbolt/domain.c | 1 +
>  drivers/usb/core/driver.c    | 1 +
>  include/linux/device/bus.h   | 4 ++++
>  4 files changed, 11 insertions(+)
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 68ea1f949daa..0cd03ac7d3b1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -544,6 +544,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  			   !drv->suppress_bind_attrs;
>  	int ret;
>  
> +	if (!dev->authorized && !dev->bus->has_probe_authorization) {
> +		dev_dbg(dev, "Device is not authorized\n");
> +		return -ENODEV;
> +	}
> +
>  	if (defer_all_probes) {
>  		/*
>  		 * Value of defer_all_probes can be set only by
> diff --git a/drivers/thunderbolt/domain.c b/drivers/thunderbolt/domain.c
> index 3e39686eff14..6de8a366b796 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -321,6 +321,7 @@ struct bus_type tb_bus_type = {
>  	.probe = tb_service_probe,
>  	.remove = tb_service_remove,
>  	.shutdown = tb_service_shutdown,
> +	.has_probe_authorization = true,
>  };
>  
>  static void tb_domain_release(struct device *dev)
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index fb476665f52d..f57b5a7a90ca 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -2028,4 +2028,5 @@ struct bus_type usb_bus_type = {
>  	.match =	usb_device_match,
>  	.uevent =	usb_uevent,
>  	.need_parent_lock =	true,
> +	.has_probe_authorization = true,
>  };
> diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
> index 062777a45a74..571a2f6e7c1d 100644
> --- a/include/linux/device/bus.h
> +++ b/include/linux/device/bus.h
> @@ -69,6 +69,9 @@ struct fwnode_handle;
>   * @lock_key:	Lock class key for use by the lock validator
>   * @need_parent_lock:	When probing or removing a device on this bus, the
>   *			device core should lock the device's parent.
> + * @has_probe_authorization: Set true to indicate to the driver-core to skip
> + *			     the authorization checks and let bus drivers
> + *			     handle it locally.
>   *
>   * A bus is a channel between the processor and one or more devices. For the
>   * purposes of the device model, all devices are connected via a bus, even if
> @@ -112,6 +115,7 @@ struct bus_type {
>  	struct lock_class_key lock_key;
>  
>  	bool need_parent_lock;
> +	bool has_probe_authorization;
>  };
>  
>  extern int __must_check bus_register(struct bus_type *bus);
> -- 
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > While the common case for device-authorization is to skip probe of
> > unauthorized devices, some buses may still want to emit a message on
> > probe failure (Thunderbolt), or base probe failures on the
> > authorization status of a related device like a parent (USB). So add
> > an option (has_probe_authorization) in struct bus_type for the bus
> > driver to own probe authorization policy.
> > 
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> 
> 
> 
> So what e.g. the PCI patch
> https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> actually proposes is a list of
> allowed drivers, not devices. Doing it at the device level
> has disadvantages, for example some devices might have a legacy
> unsafe driver, or an out of tree driver. It also does not
> address drivers that poke at hardware during init.

Doing it at a device level is the only sane way to do this.

A user needs to say "this device is allowed to be controlled by this
driver".  This is the trust model that USB has had for over a decade and
what thunderbolt also has.

> Accordingly, I think the right thing to do is to skip
> driver init for disallowed drivers, not skip probe
> for specific devices.

What do you mean by "driver init"?  module_init()?

No driver should be touching hardware in their module init call.  They
should only be touching it in the probe callback as that is the only
time they are ever allowed to talk to hardware.  Specifically the device
that has been handed to them.

If there are in-kernel PCI drivers that do not do this, they need to be
fixed today.

We don't care about out-of-tree drivers for obvious reasons that we have
no control over them.

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Michael S. Tsirkin]

On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > While the common case for device-authorization is to skip probe of
> > > unauthorized devices, some buses may still want to emit a message on
> > > probe failure (Thunderbolt), or base probe failures on the
> > > authorization status of a related device like a parent (USB). So add
> > > an option (has_probe_authorization) in struct bus_type for the bus
> > > driver to own probe authorization policy.
> > > 
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > 
> > 
> > So what e.g. the PCI patch
> > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > actually proposes is a list of
> > allowed drivers, not devices. Doing it at the device level
> > has disadvantages, for example some devices might have a legacy
> > unsafe driver, or an out of tree driver. It also does not
> > address drivers that poke at hardware during init.
> 
> Doing it at a device level is the only sane way to do this.
> 
> A user needs to say "this device is allowed to be controlled by this
> driver".  This is the trust model that USB has had for over a decade and
> what thunderbolt also has.
> 
> > Accordingly, I think the right thing to do is to skip
> > driver init for disallowed drivers, not skip probe
> > for specific devices.
> 
> What do you mean by "driver init"?  module_init()?
> 
> No driver should be touching hardware in their module init call.  They
> should only be touching it in the probe callback as that is the only
> time they are ever allowed to talk to hardware.  Specifically the device
> that has been handed to them.
> 
> If there are in-kernel PCI drivers that do not do this, they need to be
> fixed today.
> 
> We don't care about out-of-tree drivers for obvious reasons that we have
> no control over them.
> 
> thanks,
> 
> greg k-h

Well talk to Andi about it pls :)
https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 10:38:42AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > While the common case for device-authorization is to skip probe of
> > > > unauthorized devices, some buses may still want to emit a message on
> > > > probe failure (Thunderbolt), or base probe failures on the
> > > > authorization status of a related device like a parent (USB). So add
> > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > driver to own probe authorization policy.
> > > > 
> > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > 
> > > 
> > > So what e.g. the PCI patch
> > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > actually proposes is a list of
> > > allowed drivers, not devices. Doing it at the device level
> > > has disadvantages, for example some devices might have a legacy
> > > unsafe driver, or an out of tree driver. It also does not
> > > address drivers that poke at hardware during init.
> > 
> > Doing it at a device level is the only sane way to do this.
> > 
> > A user needs to say "this device is allowed to be controlled by this
> > driver".  This is the trust model that USB has had for over a decade and
> > what thunderbolt also has.
> > 
> > > Accordingly, I think the right thing to do is to skip
> > > driver init for disallowed drivers, not skip probe
> > > for specific devices.
> > 
> > What do you mean by "driver init"?  module_init()?
> > 
> > No driver should be touching hardware in their module init call.  They
> > should only be touching it in the probe callback as that is the only
> > time they are ever allowed to talk to hardware.  Specifically the device
> > that has been handed to them.
> > 
> > If there are in-kernel PCI drivers that do not do this, they need to be
> > fixed today.
> > 
> > We don't care about out-of-tree drivers for obvious reasons that we have
> > no control over them.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Well talk to Andi about it pls :)
> https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com

As Alan said, the minute you allow any driver to get into your kernel,
it can do anything it wants to.

So just don't allow drivers to be added to your kernel if you care about
these things.  The system owner has that mechanism today.

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Michael S. Tsirkin]

On Thu, Sep 30, 2021 at 04:49:23PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 10:38:42AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > > While the common case for device-authorization is to skip probe of
> > > > > unauthorized devices, some buses may still want to emit a message on
> > > > > probe failure (Thunderbolt), or base probe failures on the
> > > > > authorization status of a related device like a parent (USB). So add
> > > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > > driver to own probe authorization policy.
> > > > > 
> > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > 
> > > > 
> > > > 
> > > > So what e.g. the PCI patch
> > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > > actually proposes is a list of
> > > > allowed drivers, not devices. Doing it at the device level
> > > > has disadvantages, for example some devices might have a legacy
> > > > unsafe driver, or an out of tree driver. It also does not
> > > > address drivers that poke at hardware during init.
> > > 
> > > Doing it at a device level is the only sane way to do this.
> > > 
> > > A user needs to say "this device is allowed to be controlled by this
> > > driver".  This is the trust model that USB has had for over a decade and
> > > what thunderbolt also has.
> > > 
> > > > Accordingly, I think the right thing to do is to skip
> > > > driver init for disallowed drivers, not skip probe
> > > > for specific devices.
> > > 
> > > What do you mean by "driver init"?  module_init()?
> > > 
> > > No driver should be touching hardware in their module init call.  They
> > > should only be touching it in the probe callback as that is the only
> > > time they are ever allowed to talk to hardware.  Specifically the device
> > > that has been handed to them.
> > > 
> > > If there are in-kernel PCI drivers that do not do this, they need to be
> > > fixed today.
> > > 
> > > We don't care about out-of-tree drivers for obvious reasons that we have
> > > no control over them.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > Well talk to Andi about it pls :)
> > https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com
> 
> As Alan said, the minute you allow any driver to get into your kernel,
> it can do anything it wants to.
> 
> So just don't allow drivers to be added to your kernel if you care about
> these things.  The system owner has that mechanism today.
> 
> thanks,
> 
> greg k-h

The "it" that I referred to is the claim that no driver should be
touching hardware in their module init call. Andi seems to think
such drivers are worth working around with a special remap API.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 11:00:07AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 04:49:23PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 10:38:42AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > > > While the common case for device-authorization is to skip probe of
> > > > > > unauthorized devices, some buses may still want to emit a message on
> > > > > > probe failure (Thunderbolt), or base probe failures on the
> > > > > > authorization status of a related device like a parent (USB). So add
> > > > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > > > driver to own probe authorization policy.
> > > > > > 
> > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > > 
> > > > > 
> > > > > 
> > > > > So what e.g. the PCI patch
> > > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > > > actually proposes is a list of
> > > > > allowed drivers, not devices. Doing it at the device level
> > > > > has disadvantages, for example some devices might have a legacy
> > > > > unsafe driver, or an out of tree driver. It also does not
> > > > > address drivers that poke at hardware during init.
> > > > 
> > > > Doing it at a device level is the only sane way to do this.
> > > > 
> > > > A user needs to say "this device is allowed to be controlled by this
> > > > driver".  This is the trust model that USB has had for over a decade and
> > > > what thunderbolt also has.
> > > > 
> > > > > Accordingly, I think the right thing to do is to skip
> > > > > driver init for disallowed drivers, not skip probe
> > > > > for specific devices.
> > > > 
> > > > What do you mean by "driver init"?  module_init()?
> > > > 
> > > > No driver should be touching hardware in their module init call.  They
> > > > should only be touching it in the probe callback as that is the only
> > > > time they are ever allowed to talk to hardware.  Specifically the device
> > > > that has been handed to them.
> > > > 
> > > > If there are in-kernel PCI drivers that do not do this, they need to be
> > > > fixed today.
> > > > 
> > > > We don't care about out-of-tree drivers for obvious reasons that we have
> > > > no control over them.
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > 
> > > Well talk to Andi about it pls :)
> > > https://lore.kernel.org/r/ad1e41d1-3f4e-8982-16ea-18a3b2c04019%40linux.intel.com
> > 
> > As Alan said, the minute you allow any driver to get into your kernel,
> > it can do anything it wants to.
> > 
> > So just don't allow drivers to be added to your kernel if you care about
> > these things.  The system owner has that mechanism today.
> > 
> > thanks,
> > 
> > greg k-h
> 
> The "it" that I referred to is the claim that no driver should be
> touching hardware in their module init call. Andi seems to think
> such drivers are worth working around with a special remap API.

Andi is wrong.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Andi Kleen]

>> The "it" that I referred to is the claim that no driver should be
>> touching hardware in their module init call. Andi seems to think
>> such drivers are worth working around with a special remap API.
> Andi is wrong.

While overall it's a small percentage of the total, there are still 
quite a few drivers that do touch hardware in init functions. Sometimes 
for good reasons -- they need to do some extra probing to discover 
something that is not enumerated -- sometimes just because it's very old 
legacy code that predates the modern driver model.

The legacy drivers could be fixed, but nobody really wants to touch them 
anymore and they're impossible to test.

The drivers that probe something that is not enumerated in a standard 
way have no choice, it cannot be implemented in a different way.

So instead we're using a "firewall" the prevents these drivers from 
doing bad things by not allowing ioremap access unless opted in, and 
also do some filtering on the IO ports The device filter is still the 
primary mechanism, the ioremap filtering is just belts and suspenders 
for those odd cases.

If you want we can send an exact list, we did some analysis using a 
patched smatch tool.

-Andi

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
> 
> > > The "it" that I referred to is the claim that no driver should be
> > > touching hardware in their module init call. Andi seems to think
> > > such drivers are worth working around with a special remap API.
> > Andi is wrong.
> 
> While overall it's a small percentage of the total, there are still quite a
> few drivers that do touch hardware in init functions. Sometimes for good
> reasons -- they need to do some extra probing to discover something that is
> not enumerated -- sometimes just because it's very old legacy code that
> predates the modern driver model.

Are any of them in the kernel today?

PCI drivers should not be messing with this, we have had well over a
decade to fix that up.

> The legacy drivers could be fixed, but nobody really wants to touch them
> anymore and they're impossible to test.

Pointers to them?

> The drivers that probe something that is not enumerated in a standard way
> have no choice, it cannot be implemented in a different way.

PCI devices are not enumerated in a standard way???

> So instead we're using a "firewall" the prevents these drivers from doing
> bad things by not allowing ioremap access unless opted in, and also do some
> filtering on the IO ports The device filter is still the primary mechanism,
> the ioremap filtering is just belts and suspenders for those odd cases.

That's horrible, don't try to protect the kernel from itself.  Just fix
the drivers.

If you point me at them, I will be glad to have a look and throw some
interns on them.

But really, you all could have fixed them up by now if Intel really
cared about it :(

> If you want we can send an exact list, we did some analysis using a patched
> smatch tool.

Please do.

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Andi Kleen]

On 9/30/2021 10:23 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
>>>> The "it" that I referred to is the claim that no driver should be
>>>> touching hardware in their module init call. Andi seems to think
>>>> such drivers are worth working around with a special remap API.
>>> Andi is wrong.
>> While overall it's a small percentage of the total, there are still quite a
>> few drivers that do touch hardware in init functions. Sometimes for good
>> reasons -- they need to do some extra probing to discover something that is
>> not enumerated -- sometimes just because it's very old legacy code that
>> predates the modern driver model.
> Are any of them in the kernel today?
>
> PCI drivers should not be messing with this, we have had well over a
> decade to fix that up.


It's not just PCI driver, it's every driver that can do io port / MMIO / 
MSR / config space accesses.


Maybe read the excellent article from Jon on this:

https://lwn.net/Articles/865918/


>
>> The legacy drivers could be fixed, but nobody really wants to touch them
>> anymore and they're impossible to test.
> Pointers to them?

For example if you look over old SCSI drivers in drivers/scsi/*.c there 
is a substantial number that has a module init longer than just 
registering a driver. As a single example look at drivers/scsi/BusLogic.c

There were also quite a few platform drivers like this.


>
>> The drivers that probe something that is not enumerated in a standard way
>> have no choice, it cannot be implemented in a different way.
> PCI devices are not enumerated in a standard way???

The pci devices are enumerated in a standard way, but typically the 
driver also needs something else outside PCI that needs some other 
probing mechanism.


>
>> So instead we're using a "firewall" the prevents these drivers from doing
>> bad things by not allowing ioremap access unless opted in, and also do some
>> filtering on the IO ports The device filter is still the primary mechanism,
>> the ioremap filtering is just belts and suspenders for those odd cases.
> That's horrible, don't try to protect the kernel from itself.  Just fix
> the drivers.

I thought we had already established this last time we discussed it.

That's completely impractical. We cannot harden thousands of drivers, 
especially since it would be all wasted work since nobody will ever need 
them in virtual guests. Even if we could harden them how would such a 
work be maintained long term? Using a firewall and filtering mechanism 
is much saner for everyone.

-Andi




_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 12:15:16PM -0700, Andi Kleen wrote:
> 
> On 9/30/2021 10:23 AM, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
> > > The legacy drivers could be fixed, but nobody really wants to touch them
> > > anymore and they're impossible to test.
> > Pointers to them?
> 
> For example if you look over old SCSI drivers in drivers/scsi/*.c there is a
> substantial number that has a module init longer than just registering a
> driver. As a single example look at drivers/scsi/BusLogic.c

Great, send patches to fix them up instead of adding new infrastructure
to the kernel.  It is better to remove code than add it.  You can rip
the ISA code out of that driver and then you will not have the issue
anymore.

Or again, just add that module to the deny list and never load it from
userspace.

> There were also quite a few platform drivers like this.

Of course, platform drivers are horrible abusers of this.  Just like the
recent one submitted by Intel that would bind to any machine it was
loaded on and did not actually do any hardware detection assuming that
it owned the platform:
	https://lore.kernel.org/r/20210924213157.3584061-2-david.e.box@linux.intel.com

So yes, some drivers are horrible, it is our job to catch that and fix
it.  If you don't want to load those drivers on your system, we have
userspace solutions for that (you can have allow/deny lists there.)

> > > The drivers that probe something that is not enumerated in a standard way
> > > have no choice, it cannot be implemented in a different way.
> > PCI devices are not enumerated in a standard way???
> 
> The pci devices are enumerated in a standard way, but typically the driver
> also needs something else outside PCI that needs some other probing
> mechanism.

Like what?  What PCI drivers need outside connections to control the
hardware?

> > > So instead we're using a "firewall" the prevents these drivers from doing
> > > bad things by not allowing ioremap access unless opted in, and also do some
> > > filtering on the IO ports The device filter is still the primary mechanism,
> > > the ioremap filtering is just belts and suspenders for those odd cases.
> > That's horrible, don't try to protect the kernel from itself.  Just fix
> > the drivers.
> 
> I thought we had already established this last time we discussed it.
> 
> That's completely impractical. We cannot harden thousands of drivers,
> especially since it would be all wasted work since nobody will ever need
> them in virtual guests. Even if we could harden them how would such a work
> be maintained long term? Using a firewall and filtering mechanism is much
> saner for everyone.

I agree, you can not "harden" anything here.  That is why I asked you to
use the existing process that explicitly moves the model to userspace
where a user can say "do I want this device to be controlled by the
kernel or not" which then allows you to pick and choose what drivers you
want to have in your system.

You need to trust devices, and not worry about trusting drivers as you
yourself admit :)

The kernel's trust model is that once we bind to them, we trust almost
all device types almost explicitly.  If you wish to change that model,
that's great, but it is a much larger discussion than this tiny patchset
would require.

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Alan Stern]

On Fri, Oct 01, 2021 at 08:29:36AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 12:15:16PM -0700, Andi Kleen wrote:
> > 
> > On 9/30/2021 10:23 AM, Greg Kroah-Hartman wrote:
> > > On Thu, Sep 30, 2021 at 10:17:09AM -0700, Andi Kleen wrote:
> > > > The legacy drivers could be fixed, but nobody really wants to touch them
> > > > anymore and they're impossible to test.
> > > Pointers to them?
> > 
> > For example if you look over old SCSI drivers in drivers/scsi/*.c there is a
> > substantial number that has a module init longer than just registering a
> > driver. As a single example look at drivers/scsi/BusLogic.c
> 
> Great, send patches to fix them up instead of adding new infrastructure
> to the kernel.  It is better to remove code than add it.  You can rip
> the ISA code out of that driver and then you will not have the issue
> anymore.
> 
> Or again, just add that module to the deny list and never load it from
> userspace.
> 
> > There were also quite a few platform drivers like this.
> 
> Of course, platform drivers are horrible abusers of this.  Just like the
> recent one submitted by Intel that would bind to any machine it was
> loaded on and did not actually do any hardware detection assuming that
> it owned the platform:
> 	https://lore.kernel.org/r/20210924213157.3584061-2-david.e.box@linux.intel.com
> 
> So yes, some drivers are horrible, it is our job to catch that and fix
> it.  If you don't want to load those drivers on your system, we have
> userspace solutions for that (you can have allow/deny lists there.)
> 
> > > > The drivers that probe something that is not enumerated in a standard way
> > > > have no choice, it cannot be implemented in a different way.
> > > PCI devices are not enumerated in a standard way???
> > 
> > The pci devices are enumerated in a standard way, but typically the driver
> > also needs something else outside PCI that needs some other probing
> > mechanism.
> 
> Like what?  What PCI drivers need outside connections to control the
> hardware?
> 
> > > > So instead we're using a "firewall" the prevents these drivers from doing
> > > > bad things by not allowing ioremap access unless opted in, and also do some
> > > > filtering on the IO ports The device filter is still the primary mechanism,
> > > > the ioremap filtering is just belts and suspenders for those odd cases.
> > > That's horrible, don't try to protect the kernel from itself.  Just fix
> > > the drivers.
> > 
> > I thought we had already established this last time we discussed it.
> > 
> > That's completely impractical. We cannot harden thousands of drivers,
> > especially since it would be all wasted work since nobody will ever need
> > them in virtual guests. Even if we could harden them how would such a work
> > be maintained long term? Using a firewall and filtering mechanism is much
> > saner for everyone.
> 
> I agree, you can not "harden" anything here.  That is why I asked you to
> use the existing process that explicitly moves the model to userspace
> where a user can say "do I want this device to be controlled by the
> kernel or not" which then allows you to pick and choose what drivers you
> want to have in your system.
> 
> You need to trust devices, and not worry about trusting drivers as you
> yourself admit :)

That isn't the way they see it.  In their approach you can never 
trust any devices at all.  Therefore devices can only be allowed to 
bind to a very small set of "hardened" drivers.  Never mind how they 
decide whether or not a driver is sufficiently "hardened".

> The kernel's trust model is that once we bind to them, we trust almost
> all device types almost explicitly.  If you wish to change that model,
> that's great, but it is a much larger discussion than this tiny patchset
> would require.

Forget about trust for the moment.  Let's say the goal is to prevent 
the kernel from creating any bindings other that those in some small 
"allowed" set.  To fully specify one of the allowed bindings, you 
would have to provide both a device ID and a driver name.  But in 
practice this isn't necessary, since a device with a given ID will 
bind to only one driver in almost all cases, and hence giving just 
the device ID is enough.

So to do what they want, all that's needed is to forbid any bindings 
except where the device ID is "allowed".  Or to put it another way, 
where the device's authorized flag (which can be initialized based on 
the device ID) is set.

(The opposite approach, in which the drivers are "allowed" rather 
than the device IDs, apparently has already been discussed and 
rejected.  I'm not convinced that was a good decision, but...)

Does this seem like a fair description of the situation?

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Andi Kleen]

> Forget about trust for the moment.  Let's say the goal is to prevent
> the kernel from creating any bindings other that those in some small
> "allowed" set.  To fully specify one of the allowed bindings, you
> would have to provide both a device ID and a driver name.  But in
> practice this isn't necessary, since a device with a given ID will
> bind to only one driver in almost all cases, and hence giving just
> the device ID is enough.
>
> So to do what they want, all that's needed is to forbid any bindings
> except where the device ID is "allowed".  Or to put it another way,
> where the device's authorized flag (which can be initialized based on
> the device ID) is set.
>
> (The opposite approach, in which the drivers are "allowed" rather
> than the device IDs, apparently has already been discussed and
> rejected.  I'm not convinced that was a good decision, but...)
>
> Does this seem like a fair description of the situation?

Yes. That's roughly what the patchkit under discussion implements.


-Andi

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Alan Stern]

On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > While the common case for device-authorization is to skip probe of
> > > unauthorized devices, some buses may still want to emit a message on
> > > probe failure (Thunderbolt), or base probe failures on the
> > > authorization status of a related device like a parent (USB). So add
> > > an option (has_probe_authorization) in struct bus_type for the bus
> > > driver to own probe authorization policy.
> > > 
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > 
> > 
> > So what e.g. the PCI patch
> > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > actually proposes is a list of
> > allowed drivers, not devices. Doing it at the device level
> > has disadvantages, for example some devices might have a legacy
> > unsafe driver, or an out of tree driver. It also does not
> > address drivers that poke at hardware during init.
> 
> Doing it at a device level is the only sane way to do this.
> 
> A user needs to say "this device is allowed to be controlled by this
> driver".  This is the trust model that USB has had for over a decade and
> what thunderbolt also has.
> 
> > Accordingly, I think the right thing to do is to skip
> > driver init for disallowed drivers, not skip probe
> > for specific devices.
> 
> What do you mean by "driver init"?  module_init()?
> 
> No driver should be touching hardware in their module init call.  They
> should only be touching it in the probe callback as that is the only
> time they are ever allowed to talk to hardware.  Specifically the device
> that has been handed to them.
> 
> If there are in-kernel PCI drivers that do not do this, they need to be
> fixed today.
> 
> We don't care about out-of-tree drivers for obvious reasons that we have
> no control over them.

I don't see any point in talking about "untrusted drivers".  If a 
driver isn't trusted then it doesn't belong in your kernel.  Period.  
When you load a driver into your kernel, you are implicitly trusting 
it (aside from limitations imposed by security modules).  The code 
it contains, the module_init code in particular, runs with full 
superuser permissions.

What use is there in loading a driver but telling the kernel "I don't 
trust this driver, so don't allow it to probe any devices"?  Why not 
just blacklist it so that it never gets modprobed in the first place?

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Michael S. Tsirkin]

On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> On Thu, Sep 30, 2021 at 03:52:52PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 06:59:36AM -0400, Michael S. Tsirkin wrote:
> > > On Wed, Sep 29, 2021 at 06:05:07PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > > While the common case for device-authorization is to skip probe of
> > > > unauthorized devices, some buses may still want to emit a message on
> > > > probe failure (Thunderbolt), or base probe failures on the
> > > > authorization status of a related device like a parent (USB). So add
> > > > an option (has_probe_authorization) in struct bus_type for the bus
> > > > driver to own probe authorization policy.
> > > > 
> > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > 
> > > 
> > > So what e.g. the PCI patch
> > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=VB3pULx_GyT2NkzAogdRQJ9TKSX9A@mail.gmail.com/
> > > actually proposes is a list of
> > > allowed drivers, not devices. Doing it at the device level
> > > has disadvantages, for example some devices might have a legacy
> > > unsafe driver, or an out of tree driver. It also does not
> > > address drivers that poke at hardware during init.
> > 
> > Doing it at a device level is the only sane way to do this.
> > 
> > A user needs to say "this device is allowed to be controlled by this
> > driver".  This is the trust model that USB has had for over a decade and
> > what thunderbolt also has.
> > 
> > > Accordingly, I think the right thing to do is to skip
> > > driver init for disallowed drivers, not skip probe
> > > for specific devices.
> > 
> > What do you mean by "driver init"?  module_init()?
> > 
> > No driver should be touching hardware in their module init call.  They
> > should only be touching it in the probe callback as that is the only
> > time they are ever allowed to talk to hardware.  Specifically the device
> > that has been handed to them.
> > 
> > If there are in-kernel PCI drivers that do not do this, they need to be
> > fixed today.
> > 
> > We don't care about out-of-tree drivers for obvious reasons that we have
> > no control over them.
> 
> I don't see any point in talking about "untrusted drivers".  If a 
> driver isn't trusted then it doesn't belong in your kernel.  Period.  
> When you load a driver into your kernel, you are implicitly trusting 
> it (aside from limitations imposed by security modules).  The code 
> it contains, the module_init code in particular, runs with full 
> superuser permissions.
> 
> What use is there in loading a driver but telling the kernel "I don't 
> trust this driver, so don't allow it to probe any devices"?  Why not 
> just blacklist it so that it never gets modprobed in the first place?
> 
> Alan Stern

When the driver is built-in, it seems useful to be able to block it
without rebuilding the kernel. This is just flipping it around
and using an allow-list for cases where you want to severly
limit the available functionality.


-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Alan Stern]

On Thu, Sep 30, 2021 at 10:48:54AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > I don't see any point in talking about "untrusted drivers".  If a 
> > driver isn't trusted then it doesn't belong in your kernel.  Period.  
> > When you load a driver into your kernel, you are implicitly trusting 
> > it (aside from limitations imposed by security modules).  The code 
> > it contains, the module_init code in particular, runs with full 
> > superuser permissions.
> > 
> > What use is there in loading a driver but telling the kernel "I don't 
> > trust this driver, so don't allow it to probe any devices"?  Why not 
> > just blacklist it so that it never gets modprobed in the first place?
> > 
> > Alan Stern
> 
> When the driver is built-in, it seems useful to be able to block it
> without rebuilding the kernel. This is just flipping it around
> and using an allow-list for cases where you want to severly
> limit the available functionality.

Does this make sense?

The only way to tell the kernel to block a built-in driver is by 
using some boot-command-line option.  Otherwise the driver's init 
code will run before you have a chance to tell the kernel anything at 
all.

So if you change your mind about whether a driver should be blocked, 
all you have to do is remove the blocking option from the command 
line and reboot.  No kernel rebuild is necessary.

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Michael S. Tsirkin]

On Thu, Sep 30, 2021 at 11:32:41AM -0400, Alan Stern wrote:
> On Thu, Sep 30, 2021 at 10:48:54AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > > I don't see any point in talking about "untrusted drivers".  If a 
> > > driver isn't trusted then it doesn't belong in your kernel.  Period.  
> > > When you load a driver into your kernel, you are implicitly trusting 
> > > it (aside from limitations imposed by security modules).  The code 
> > > it contains, the module_init code in particular, runs with full 
> > > superuser permissions.
> > > 
> > > What use is there in loading a driver but telling the kernel "I don't 
> > > trust this driver, so don't allow it to probe any devices"?  Why not 
> > > just blacklist it so that it never gets modprobed in the first place?
> > > 
> > > Alan Stern
> > 
> > When the driver is built-in, it seems useful to be able to block it
> > without rebuilding the kernel. This is just flipping it around
> > and using an allow-list for cases where you want to severly
> > limit the available functionality.
> 
> Does this make sense?
> 
> The only way to tell the kernel to block a built-in driver is by 
> using some boot-command-line option.  Otherwise the driver's init 
> code will run before you have a chance to tell the kernel anything at 
> all.
> 
> So if you change your mind about whether a driver should be blocked, 
> all you have to do is remove the blocking option from the command 
> line and reboot.  No kernel rebuild is necessary.
> 
> Alan Stern

Right.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Michael S. Tsirkin]

On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> I don't see any point in talking about "untrusted drivers".  If a 
> driver isn't trusted then it doesn't belong in your kernel.  Period.  
> When you load a driver into your kernel, you are implicitly trusting 
> it (aside from limitations imposed by security modules).

Trusting it to do what? Historically a ton of drivers did not
validate input from devices they drive. Most still don't.

> The code 
> it contains, the module_init code in particular, runs with full 
> superuser permissions.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Alan Stern]

On Thu, Sep 30, 2021 at 10:58:07AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > I don't see any point in talking about "untrusted drivers".  If a 
> > driver isn't trusted then it doesn't belong in your kernel.  Period.  
> > When you load a driver into your kernel, you are implicitly trusting 
> > it (aside from limitations imposed by security modules).
> 
> Trusting it to do what? Historically a ton of drivers did not
> validate input from devices they drive. Most still don't.

Trusting it to behave properly (i.e., not destroy your system, among 
other things).

The fact that many drivers haven't been trustworthy is beside the 
point.  By loading them into your kernel, you are trusting them 
regardless.  In the end, you may regret having done so.  :-(

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Michael S. Tsirkin]

On Thu, Sep 30, 2021 at 11:35:09AM -0400, Alan Stern wrote:
> On Thu, Sep 30, 2021 at 10:58:07AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Sep 30, 2021 at 10:43:05AM -0400, Alan Stern wrote:
> > > I don't see any point in talking about "untrusted drivers".  If a 
> > > driver isn't trusted then it doesn't belong in your kernel.  Period.  
> > > When you load a driver into your kernel, you are implicitly trusting 
> > > it (aside from limitations imposed by security modules).
> > 
> > Trusting it to do what? Historically a ton of drivers did not
> > validate input from devices they drive. Most still don't.
> 
> Trusting it to behave properly (i.e., not destroy your system, among 
> other things).

I don't think the current mitigations under discussion here are about
keeping the system working. In fact most encrypted VM configs tend to
stop booting as a preferred way to handle security issues.

> The fact that many drivers haven't been trustworthy is beside the 
> point.  By loading them into your kernel, you are trusting them 
> regardless.  In the end, you may regret having done so.  :-(
> 
> Alan Stern



-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Andi Kleen]

> I don't think the current mitigations under discussion here are about
> keeping the system working. In fact most encrypted VM configs tend to
> stop booting as a preferred way to handle security issues.

Maybe we should avoid the "trusted" term here. We're only really using 
it because USB is using it and we're now using a common framework like 
Greg requested. But I don't think it's the right way to think about it.

We usually call the drivers "hardened". The requirement for a hardened 
driver is that all interactions through MMIO/port/config space IO/MSRs 
are sanitized and do not cause memory safety issues or other information 
leaks. Other than that there is no requirement on the functionality. In 
particular DOS is ok since a malicious hypervisor can decide to not run 
the guest at any time anyways.

Someone loading an malicious driver inside the guest would be out of 
scope. If an attacker can do that inside the guest you already violated 
the security mechanisms and there are likely easier ways to take over 
the guest or leak data.

The goal of the device filter mechanism is to prevent loading unhardened 
drivers that could be exploited without them being themselves malicious.


-Andi


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Alan Stern]

On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
> 
> > I don't think the current mitigations under discussion here are about
> > keeping the system working. In fact most encrypted VM configs tend to
> > stop booting as a preferred way to handle security issues.
> 
> Maybe we should avoid the "trusted" term here. We're only really using it
> because USB is using it and we're now using a common framework like Greg
> requested. But I don't think it's the right way to think about it.
> 
> We usually call the drivers "hardened". The requirement for a hardened
> driver is that all interactions through MMIO/port/config space IO/MSRs are
> sanitized and do not cause memory safety issues or other information leaks.
> Other than that there is no requirement on the functionality. In particular
> DOS is ok since a malicious hypervisor can decide to not run the guest at
> any time anyways.
> 
> Someone loading an malicious driver inside the guest would be out of scope.
> If an attacker can do that inside the guest you already violated the
> security mechanisms and there are likely easier ways to take over the guest
> or leak data.
> 
> The goal of the device filter mechanism is to prevent loading unhardened
> drivers that could be exploited without them being themselves malicious.

If all you want to do is prevent someone from loading a bunch of 
drivers that you have identified as unhardened, why not just use a 
modprobe blacklist?  Am I missing something?

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Dan Williams]

On Thu, Sep 30, 2021 at 1:44 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
> >
> > > I don't think the current mitigations under discussion here are about
> > > keeping the system working. In fact most encrypted VM configs tend to
> > > stop booting as a preferred way to handle security issues.
> >
> > Maybe we should avoid the "trusted" term here. We're only really using it
> > because USB is using it and we're now using a common framework like Greg
> > requested. But I don't think it's the right way to think about it.
> >
> > We usually call the drivers "hardened". The requirement for a hardened
> > driver is that all interactions through MMIO/port/config space IO/MSRs are
> > sanitized and do not cause memory safety issues or other information leaks.
> > Other than that there is no requirement on the functionality. In particular
> > DOS is ok since a malicious hypervisor can decide to not run the guest at
> > any time anyways.
> >
> > Someone loading an malicious driver inside the guest would be out of scope.
> > If an attacker can do that inside the guest you already violated the
> > security mechanisms and there are likely easier ways to take over the guest
> > or leak data.
> >
> > The goal of the device filter mechanism is to prevent loading unhardened
> > drivers that could be exploited without them being themselves malicious.
>
> If all you want to do is prevent someone from loading a bunch of
> drivers that you have identified as unhardened, why not just use a
> modprobe blacklist?  Am I missing something?

modules != drivers (i.e. multi-driver modules are a thing) and builtin
modules do not adhere to modprobe policy.

There is also a desire to be able to support a single kernel image
across hosts and guests. So, if you were going to say, "just compile
all unnecessary drivers as modules" that defeats the common kernel
image goal. For confidential computing the expectation is that the
necessary device set is small. As you can see in the patches in this
case it's just a few lines of PCI ids and a hack to the virtio bus to
achieve the goal of disabling all extraneous devices by default.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Alan Stern]

On Thu, Sep 30, 2021 at 01:52:59PM -0700, Dan Williams wrote:
> On Thu, Sep 30, 2021 at 1:44 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
> > >
> > > > I don't think the current mitigations under discussion here are about
> > > > keeping the system working. In fact most encrypted VM configs tend to
> > > > stop booting as a preferred way to handle security issues.
> > >
> > > Maybe we should avoid the "trusted" term here. We're only really using it
> > > because USB is using it and we're now using a common framework like Greg
> > > requested. But I don't think it's the right way to think about it.
> > >
> > > We usually call the drivers "hardened". The requirement for a hardened
> > > driver is that all interactions through MMIO/port/config space IO/MSRs are
> > > sanitized and do not cause memory safety issues or other information leaks.
> > > Other than that there is no requirement on the functionality. In particular
> > > DOS is ok since a malicious hypervisor can decide to not run the guest at
> > > any time anyways.
> > >
> > > Someone loading an malicious driver inside the guest would be out of scope.
> > > If an attacker can do that inside the guest you already violated the
> > > security mechanisms and there are likely easier ways to take over the guest
> > > or leak data.
> > >
> > > The goal of the device filter mechanism is to prevent loading unhardened
> > > drivers that could be exploited without them being themselves malicious.
> >
> > If all you want to do is prevent someone from loading a bunch of
> > drivers that you have identified as unhardened, why not just use a
> > modprobe blacklist?  Am I missing something?
> 
> modules != drivers (i.e. multi-driver modules are a thing) and builtin
> modules do not adhere to modprobe policy.
> 
> There is also a desire to be able to support a single kernel image
> across hosts and guests. So, if you were going to say, "just compile
> all unnecessary drivers as modules" that defeats the common kernel
> image goal. For confidential computing the expectation is that the
> necessary device set is small. As you can see in the patches in this
> case it's just a few lines of PCI ids and a hack to the virtio bus to
> achieve the goal of disabling all extraneous devices by default.



If your goal is to prevent some unwanted _drivers_ from operating -- 
or all but a few desired drivers, as the case may be -- why extend 
the "authorized" API to all _devices_?  Why not instead develop a 
separate API (but of similar form) for drivers?

Wouldn't that make more sense?  It corresponds a lot more directly 
with what you say you want to accomplish.

What would you do in the theoretical case where two separate drivers 
can manage the same device, but one of them is desired (or hardened) 
and the other isn't?

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Dan Williams]

On Thu, Sep 30, 2021 at 6:41 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, Sep 30, 2021 at 01:52:59PM -0700, Dan Williams wrote:
> > On Thu, Sep 30, 2021 at 1:44 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Thu, Sep 30, 2021 at 12:23:36PM -0700, Andi Kleen wrote:
> > > >
> > > > > I don't think the current mitigations under discussion here are about
> > > > > keeping the system working. In fact most encrypted VM configs tend to
> > > > > stop booting as a preferred way to handle security issues.
> > > >
> > > > Maybe we should avoid the "trusted" term here. We're only really using it
> > > > because USB is using it and we're now using a common framework like Greg
> > > > requested. But I don't think it's the right way to think about it.
> > > >
> > > > We usually call the drivers "hardened". The requirement for a hardened
> > > > driver is that all interactions through MMIO/port/config space IO/MSRs are
> > > > sanitized and do not cause memory safety issues or other information leaks.
> > > > Other than that there is no requirement on the functionality. In particular
> > > > DOS is ok since a malicious hypervisor can decide to not run the guest at
> > > > any time anyways.
> > > >
> > > > Someone loading an malicious driver inside the guest would be out of scope.
> > > > If an attacker can do that inside the guest you already violated the
> > > > security mechanisms and there are likely easier ways to take over the guest
> > > > or leak data.
> > > >
> > > > The goal of the device filter mechanism is to prevent loading unhardened
> > > > drivers that could be exploited without them being themselves malicious.
> > >
> > > If all you want to do is prevent someone from loading a bunch of
> > > drivers that you have identified as unhardened, why not just use a
> > > modprobe blacklist?  Am I missing something?
> >
> > modules != drivers (i.e. multi-driver modules are a thing) and builtin
> > modules do not adhere to modprobe policy.
> >
> > There is also a desire to be able to support a single kernel image
> > across hosts and guests. So, if you were going to say, "just compile
> > all unnecessary drivers as modules" that defeats the common kernel
> > image goal. For confidential computing the expectation is that the
> > necessary device set is small. As you can see in the patches in this
> > case it's just a few lines of PCI ids and a hack to the virtio bus to
> > achieve the goal of disabling all extraneous devices by default.
>
>
>
> If your goal is to prevent some unwanted _drivers_ from operating --
> or all but a few desired drivers, as the case may be -- why extend
> the "authorized" API to all _devices_?  Why not instead develop a
> separate API (but of similar form) for drivers?
>
> Wouldn't that make more sense?  It corresponds a lot more directly
> with what you say you want to accomplish.

This was v1. v1 was NAKd [1] [2]:

[1]: https://lore.kernel.org/all/YQwpa+LAYt7YZ5dh@kroah.com/
[2]: https://lore.kernel.org/all/YQzDqm6FOezM6Rnu@kroah.com/

> What would you do in the theoretical case where two separate drivers
> can manage the same device, but one of them is desired (or hardened)
> and the other isn't?

Allow for user override, just like we do today for new_id, remove_id,
bind, and unbind  when default driver policy is insufficient.

echo 1 > /sys/bus/$bus/devices/$device/authorized
echo $device > /sys/bus/$bus/drivers/$desired_driver/bind

The device filter is really only necessary to bootstrap until you can
run override policy scripts. The driver firewall approach was overkill
in that regard.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices

[Andi Kleen]

> If all you want to do is prevent someone from loading a bunch of
> drivers that you have identified as unhardened, why not just use a
> modprobe blacklist?

That wouldn't help for builtin drivers, we cannot control initcalls.

This LWN article has more details on the background.

https://lwn.net/Articles/865918/

-Andi


> Am I missing something?
>
> Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Michael S. Tsirkin]

On Wed, Sep 29, 2021 at 06:05:09PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Confidential guest platforms like TDX have a requirement to allow
> only trusted devices. By default the confidential-guest core will
> arrange for all devices to default to unauthorized (via
> dev_default_authorization) in device_initialize(). Since virtio
> driver is already hardened against the attack from the un-trusted host,
> override the confidential computing default unauthorized state
> 
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Architecturally this all looks backwards. IIUC nothing about virtio
makes it authorized or trusted. The driver is hardened,
true, but this should be set at the driver not the device level.
And in particular, not all virtio drivers are hardened -
I think at this point blk and scsi drivers have been hardened - so
treating them all the same looks wrong.

> ---
>  drivers/virtio/virtio.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 588e02fb91d3..377b0ccdc503 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -5,6 +5,8 @@
>  #include <linux/module.h>
>  #include <linux/idr.h>
>  #include <linux/of.h>
> +#include <linux/cc_platform.h>
> +#include <linux/device.h>
>  #include <uapi/linux/virtio_ids.h>
>  
>  /* Unique numbering for virtio devices. */
> @@ -390,6 +392,13 @@ int register_virtio_device(struct virtio_device *dev)
>  	dev->config_enabled = false;
>  	dev->config_change_pending = false;
>  
> +	/*
> +	 * For Confidential guest (like TDX), virtio devices are
> +	 * trusted. So set authorized status as true.
> +	 */
> +	if (cc_platform_has(CC_ATTR_GUEST_DEVICE_FILTER))
> +		dev->dev.authorized = true;
> +
>  	/* We always start by resetting the device, in case a previous
>  	 * driver messed it up.  This also tests that code path a little. */
>  	dev->config->reset(dev);
> -- 
> 2.25.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Dan Williams]

On Thu, Sep 30, 2021 at 4:03 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Sep 29, 2021 at 06:05:09PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Confidential guest platforms like TDX have a requirement to allow
> > only trusted devices. By default the confidential-guest core will
> > arrange for all devices to default to unauthorized (via
> > dev_default_authorization) in device_initialize(). Since virtio
> > driver is already hardened against the attack from the un-trusted host,
> > override the confidential computing default unauthorized state
> >
> > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> Architecturally this all looks backwards. IIUC nothing about virtio
> makes it authorized or trusted. The driver is hardened,
> true, but this should be set at the driver not the device level.

That's was my initial reaction to this proposal as well, and I ended
up leading Sathya astray from what Greg wanted. Greg rightly points
out that the "authorized" attribute from USB and Thunderbolt already
exists [1] [2]. So the choice is find an awkward way to mix driver
trust with existing bus-local "authorized" mechanisms, or promote the
authorized capability to the driver-core. This patch set implements
the latter to keep the momentum on the already shipping design scheme
to not add to the driver-core maintenance burden.

[1]: https://lore.kernel.org/all/YQuaJ78y8j1UmBoz@kroah.com/
[2]: https://lore.kernel.org/all/YQzF%2FutgrJfbZuHh@kroah.com/

> And in particular, not all virtio drivers are hardened -
> I think at this point blk and scsi drivers have been hardened - so
> treating them all the same looks wrong.

My understanding was that they have been audited, Sathya?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 06:36:18AM -0700, Dan Williams wrote:
> On Thu, Sep 30, 2021 at 4:03 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Sep 29, 2021 at 06:05:09PM -0700, Kuppuswamy Sathyanarayanan wrote:
> > > Confidential guest platforms like TDX have a requirement to allow
> > > only trusted devices. By default the confidential-guest core will
> > > arrange for all devices to default to unauthorized (via
> > > dev_default_authorization) in device_initialize(). Since virtio
> > > driver is already hardened against the attack from the un-trusted host,
> > > override the confidential computing default unauthorized state
> > >
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >
> > Architecturally this all looks backwards. IIUC nothing about virtio
> > makes it authorized or trusted. The driver is hardened,
> > true, but this should be set at the driver not the device level.
> 
> That's was my initial reaction to this proposal as well, and I ended
> up leading Sathya astray from what Greg wanted. Greg rightly points
> out that the "authorized" attribute from USB and Thunderbolt already
> exists [1] [2]. So the choice is find an awkward way to mix driver
> trust with existing bus-local "authorized" mechanisms, or promote the
> authorized capability to the driver-core. This patch set implements
> the latter to keep the momentum on the already shipping design scheme
> to not add to the driver-core maintenance burden.
> 
> [1]: https://lore.kernel.org/all/YQuaJ78y8j1UmBoz@kroah.com/
> [2]: https://lore.kernel.org/all/YQzF%2FutgrJfbZuHh@kroah.com/
> 
> > And in particular, not all virtio drivers are hardened -
> > I think at this point blk and scsi drivers have been hardened - so
> > treating them all the same looks wrong.
> 
> My understanding was that they have been audited, Sathya?

Please define "audited" and "trusted" here.

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Michael S. Tsirkin]

On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 9/30/21 6:36 AM, Dan Williams wrote:
> > > And in particular, not all virtio drivers are hardened -
> > > I think at this point blk and scsi drivers have been hardened - so
> > > treating them all the same looks wrong.
> > My understanding was that they have been audited, Sathya?
> 
> Yes, AFAIK, it has been audited. Andi also submitted some patches
> related to it. Andi, can you confirm.
> 
> We also authorize the virtio at PCI ID level. And currently we allow
> console, block and net virtio PCI devices.
> 
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_NET) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_BLOCK) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_CONSOLE) },

Presumably modern IDs should be allowed too?

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 9/30/21 6:36 AM, Dan Williams wrote:
> > > And in particular, not all virtio drivers are hardened -
> > > I think at this point blk and scsi drivers have been hardened - so
> > > treating them all the same looks wrong.
> > My understanding was that they have been audited, Sathya?
> 
> Yes, AFAIK, it has been audited. Andi also submitted some patches
> related to it. Andi, can you confirm.

What is the official definition of "audited"?

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Andi Kleen]

On 9/30/2021 12:04 PM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
>> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan 
>> wrote:
>>>
>>>
>>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>>> And in particular, not all virtio drivers are hardened -
>>>>> I think at this point blk and scsi drivers have been hardened - so
>>>>> treating them all the same looks wrong.
>>>> My understanding was that they have been audited, Sathya?
>>>
>>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>>> related to it. Andi, can you confirm.
>>
>> What is the official definition of "audited"?
>
>
> In our case (Confidential Computing platform), the host is an un-trusted
> entity. So any interaction with host from the drivers will have to be
> protected against the possible attack from the host. For example, if we
> are accessing a memory based on index value received from host, we have
> to make sure it does not lead to out of bound access or when sharing the
> memory with the host, we need to make sure only the required region is
> shared with the host and the memory is un-shared after use properly.
>
> Elena can share more details, but it was achieved with static analysis
> and fuzzing. Here is a presentation she is sharing about the work at the
> Linux Security Summit:
> https://static.sched.com/hosted_files/lssna2021/b6/LSS-HardeningLinuxGuestForCCC.pdf 
>
>
> Andi, can talk more about the specific driver changes that came out of 
> this
> effort.

The original virtio was quite easy to exploit because it put its free 
list into the shared ring buffer.

We had a patchkit to harden virtio originally, but after some discussion 
we instead switched to Jason Wang's patchkit to move the virtio metadata 
into protected memory, which fixed near all of the issues. These patches 
have been already merged. There is one additional patch to limit the 
virtio modes.

There's an ongoing effort to audit (mostly finished I believe) and fuzz 
the three virtio drivers (fuzzing is still ongoing).

There was also a range of changes outside virtio for code outside the 
device model. Most of it was just disabling it though.

-Andi

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Greg Kroah-Hartman]

On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> 
> 
> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
> > On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > 
> > > 
> > > On 9/30/21 6:36 AM, Dan Williams wrote:
> > > > > And in particular, not all virtio drivers are hardened -
> > > > > I think at this point blk and scsi drivers have been hardened - so
> > > > > treating them all the same looks wrong.
> > > > My understanding was that they have been audited, Sathya?
> > > 
> > > Yes, AFAIK, it has been audited. Andi also submitted some patches
> > > related to it. Andi, can you confirm.
> > 
> > What is the official definition of "audited"?
> 
> 
> In our case (Confidential Computing platform), the host is an un-trusted
> entity. So any interaction with host from the drivers will have to be
> protected against the possible attack from the host. For example, if we
> are accessing a memory based on index value received from host, we have
> to make sure it does not lead to out of bound access or when sharing the
> memory with the host, we need to make sure only the required region is
> shared with the host and the memory is un-shared after use properly.

You have not defined the term "audited" here at all in any way that can
be reviewed or verified by anyone from what I can tell.

You have only described a new model that you wish the kernel to run in,
one in which it does not trust the hardware at all.  That is explicitly
NOT what the kernel has been designed for so far, and if you wish to
change that, lots of things need to be done outside of simply running
some fuzzers on a few random drivers.

For one example, how do you ensure that the memory you are reading from
hasn't been modified by the host between writing to it the last time you
did?  Do you have a list of specific drivers and kernel options that you
feel you now "trust"?  If so, how long does that trust last for?  Until
someonen else modifies that code?  What about modifications to functions
that your "audited" code touches?  Who is doing this auditing?  How do
you know the auditing has been done correctly?  Who has reviewed and
audited the tools that are doing the auditing?  Where is the
specification that has been agreed on how the auditing must be done?
And so on...

I feel like there are a lot of different things all being mixed up here
into one "oh we want this to happen!" type of thread.  Please let's just
stick to the one request that I had here, which was to move the way that
busses are allowed to authorize the devices they wish to control into a
generic way instead of being bus-specific logic.

Any requests outside of that type of functionality are just that,
outside the scope of this patchset and should get their own patch series
and discussion.

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Andi Kleen]

On 10/1/2021 12:03 AM, Greg Kroah-Hartman wrote:
> On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>
>> On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
>>> On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
>>>>
>>>> On 9/30/21 6:36 AM, Dan Williams wrote:
>>>>>> And in particular, not all virtio drivers are hardened -
>>>>>> I think at this point blk and scsi drivers have been hardened - so
>>>>>> treating them all the same looks wrong.
>>>>> My understanding was that they have been audited, Sathya?
>>>> Yes, AFAIK, it has been audited. Andi also submitted some patches
>>>> related to it. Andi, can you confirm.
>>> What is the official definition of "audited"?
>>
>> In our case (Confidential Computing platform), the host is an un-trusted
>> entity. So any interaction with host from the drivers will have to be
>> protected against the possible attack from the host. For example, if we
>> are accessing a memory based on index value received from host, we have
>> to make sure it does not lead to out of bound access or when sharing the
>> memory with the host, we need to make sure only the required region is
>> shared with the host and the memory is un-shared after use properly.
> You have not defined the term "audited" here at all in any way that can
> be reviewed or verified by anyone from what I can tell.
>
> You have only described a new model that you wish the kernel to run in,
> one in which it does not trust the hardware at all.  That is explicitly
> NOT what the kernel has been designed for so far,

It has been already done for a few USB/TB drivers, but yes not for the 
majority of the kernel.

>   and if you wish to
> change that, lots of things need to be done outside of simply running
> some fuzzers on a few random drivers.

The goal is to do similar work as USB/TB did, but do it for a small set 
of virtio drivers and use a custom allow list for those for the specific 
secure guest cases.

(there are some other goals, but let's not discuss them here for now)


>
> For one example, how do you ensure that the memory you are reading from
> hasn't been modified by the host between writing to it the last time you
> did?

It's similar techniques as we do on user space accesses. For example if 
you bound check some value the code needs to ensure it is cached in 
private memory, not reread from MMIO or shared memory. Of course that's 
a good idea anyways for performance because MMIO is slow.

In the concrete cases of virtio the main problem was the free list in 
shared memory, but that has been addressed now.



>   Do you have a list of specific drivers and kernel options that you
> feel you now "trust"?

For TDX it's currently only virtio net/block/console

But we expect this list to grow slightly over time, but not at a high 
rate (so hopefully <10)


> If so, how long does that trust last for?  Until
> someonen else modifies that code?  What about modifications to functions
> that your "audited" code touches?  Who is doing this auditing?  How do
> you know the auditing has been done correctly?  Who has reviewed and
> audited the tools that are doing the auditing?  Where is the
> specification that has been agreed on how the auditing must be done?
> And so on...

Well, I mean we already have a similar situation with user space APIs. 
So it's not a new problem. For those we've done it for many years, with 
audits and extra fuzzing.

There are people working on the audit and fuzzing today. How exactly it 
will be ensured long term is still be worked out, but I expect we can 
work out something.

>
> I feel like there are a lot of different things all being mixed up here
> into one "oh we want this to happen!" type of thread.



Agreed. The thread ended up about a lot of stuff which is outside the 
scope of the patches.

>    Please let's just
> stick to the one request that I had here, which was to move the way that
> busses are allowed to authorize the devices they wish to control into a
> generic way instead of being bus-specific logic.
>
> Any requests outside of that type of functionality are just that,
> outside the scope of this patchset and should get their own patch series
> and discussion.


Yes that's the intention. This patch kit is only about controlling what 
devices can enumerate.

Also please let's avoid the "trusted" term. It's really misleading and 
confusing in the context of confidential computing.


-Andi

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Michael S. Tsirkin]

On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> >   Do you have a list of specific drivers and kernel options that you
> > feel you now "trust"?
> 
> For TDX it's currently only virtio net/block/console
> 
> But we expect this list to grow slightly over time, but not at a high rate
> (so hopefully <10)

Well there are already >10 virtio drivers and I think it's reasonable
that all of these will be used with encrypted guests. The list will
grow.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Greg Kroah-Hartman]

On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > >   Do you have a list of specific drivers and kernel options that you
> > > feel you now "trust"?
> > 
> > For TDX it's currently only virtio net/block/console
> > 
> > But we expect this list to grow slightly over time, but not at a high rate
> > (so hopefully <10)
> 
> Well there are already >10 virtio drivers and I think it's reasonable
> that all of these will be used with encrypted guests. The list will
> grow.

What is keeping "all" drivers from being on this list?  How exactly are
you determining what should, and should not, be allowed?  How can
drivers move on, or off, of it over time?

And why not just put all of that into userspace and have it pick and
choose?  That should be the end-goal here, you don't want to encode
policy like this in the kernel, right?

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Andi Kleen]

On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
>> On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
>>>>    Do you have a list of specific drivers and kernel options that you
>>>> feel you now "trust"?
>>> For TDX it's currently only virtio net/block/console
>>>
>>> But we expect this list to grow slightly over time, but not at a high rate
>>> (so hopefully <10)
>> Well there are already >10 virtio drivers and I think it's reasonable
>> that all of these will be used with encrypted guests. The list will
>> grow.
> What is keeping "all" drivers from being on this list?

It would be too much work to harden them all, and it would be pointless 
because all these drivers are never legitimately needed in a virtualized 
environment which only virtualize a very small number of devices.

>   How exactly are
> you determining what should, and should not, be allowed?

Everything that has had reasonable effort at hardening can be added. But 
if someone proposes to add a driver that should trigger additional 
scrutiny in code review. We should also request them to do some fuzzing.

It's a bit similar to someone trying to add a new syscall interface. 
That also triggers much additional scrutiny for good reasons and people 
start fuzzing it.


>    How can
> drivers move on, or off, of it over time?

Adding something is submitting a patch to the allow list.

I'm not sure the "off" case would happen, unless the driver is 
completely removed, or maybe it has some unfixable security problem. But 
that is all rather unlikely.


>
> And why not just put all of that into userspace and have it pick and
> choose?  That should be the end-goal here, you don't want to encode
> policy like this in the kernel, right?

How would user space know what drivers have been hardened? This is 
really something that the kernel needs to determine. I don't think we 
can outsource it to anyone else.

Also BTW of course user space can still override it, but really the 
defaults should be a kernel policy.

There's also the additional problem that one of the goals of 
confidential guest is to just move existing guest virtual images into 
them without much changes. So it's better for such a case if as much as 
possible of the policy is in the kernel. But that's more a secondary 
consideration, the first point is really the important part.


-Andi

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Greg Kroah-Hartman]

On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote:
> 
> On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > > > >    Do you have a list of specific drivers and kernel options that you
> > > > > feel you now "trust"?
> > > > For TDX it's currently only virtio net/block/console
> > > > 
> > > > But we expect this list to grow slightly over time, but not at a high rate
> > > > (so hopefully <10)
> > > Well there are already >10 virtio drivers and I think it's reasonable
> > > that all of these will be used with encrypted guests. The list will
> > > grow.
> > What is keeping "all" drivers from being on this list?
> 
> It would be too much work to harden them all, and it would be pointless
> because all these drivers are never legitimately needed in a virtualized
> environment which only virtualize a very small number of devices.

Why would you not want to properly review and fix up all kernel drivers?
That feels like you are being lazy.

What exactly are you meaning by "harden"?  Why isn't it automated?  Who
is doing this work?  Where is it being done?

Come on, you have a small number of virtio drivers, to somehow say that
they are now divided up into trusted/untrusted feels very very odd.

Just do the real work here, everyone will benefit, including yourself.

> >   How exactly are
> > you determining what should, and should not, be allowed?
> 
> Everything that has had reasonable effort at hardening can be added. But if
> someone proposes to add a driver that should trigger additional scrutiny in
> code review. We should also request them to do some fuzzing.

You can provide that fuzzing right now, why isn't syzbot running on
these interfaces today?

And again, what _exactly_ do you all mean by "hardening" that has
happened?  Where is that documented and who did that work?

> > And why not just put all of that into userspace and have it pick and
> > choose?  That should be the end-goal here, you don't want to encode
> > policy like this in the kernel, right?
> 
> How would user space know what drivers have been hardened? This is really
> something that the kernel needs to determine. I don't think we can outsource
> it to anyone else.

It would "know" just as well as you know today in the kernel.  There is
no difference here.

Just do the real work here, and "harden" all of the virtio drivers
please.  What prevents that?

> Also BTW of course user space can still override it, but really the defaults
> should be a kernel policy.
> 
> There's also the additional problem that one of the goals of confidential
> guest is to just move existing guest virtual images into them without much
> changes. So it's better for such a case if as much as possible of the policy
> is in the kernel. But that's more a secondary consideration, the first point
> is really the important part.

Where exactly are all of these "goals" and "requirements" documented and
who is defining them and forcing them on us without actually doing any
of the work involved?

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Michael S. Tsirkin]

On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote:
> 
> On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > > > >    Do you have a list of specific drivers and kernel options that you
> > > > > feel you now "trust"?
> > > > For TDX it's currently only virtio net/block/console
> > > > 
> > > > But we expect this list to grow slightly over time, but not at a high rate
> > > > (so hopefully <10)
> > > Well there are already >10 virtio drivers and I think it's reasonable
> > > that all of these will be used with encrypted guests. The list will
> > > grow.
> > What is keeping "all" drivers from being on this list?
> 
> It would be too much work to harden them all, and it would be pointless
> because all these drivers are never legitimately needed in a virtualized
> environment which only virtualize a very small number of devices.
> 
> >   How exactly are
> > you determining what should, and should not, be allowed?
> 
> Everything that has had reasonable effort at hardening can be added. But if
> someone proposes to add a driver that should trigger additional scrutiny in
> code review. We should also request them to do some fuzzing.

Looks like out of tree modules get a free pass then.
Which is exactly the reverse of what it should be,
people who spent the time to get their drivers into the kernel
expect that if kernel decides to change some API their
driver is automatically updated. This was always the social
contract, was it not?

> It's a bit similar to someone trying to add a new syscall interface. That
> also triggers much additional scrutiny for good reasons and people start
> fuzzing it.
> 
> 
> >    How can
> > drivers move on, or off, of it over time?
> 
> Adding something is submitting a patch to the allow list.
> 
> I'm not sure the "off" case would happen, unless the driver is completely
> removed, or maybe it has some unfixable security problem. But that is all
> rather unlikely.
> 
> 
> > 
> > And why not just put all of that into userspace and have it pick and
> > choose?  That should be the end-goal here, you don't want to encode
> > policy like this in the kernel, right?
> 
> How would user space know what drivers have been hardened? This is really
> something that the kernel needs to determine. I don't think we can outsource
> it to anyone else.

IIUC userspace is the distro. It can also do more than a binary on/off,
e.g. it can decide "only virtio", "no out of tree drivers".
A distro can also ship configs with a specific features
enabled/disabled. E.g. I can see where some GPU drivers will be
included by some distros since they are so useful, and excluded
by others since they are so big and hard to audit.
I don't see how the kernel can reasonably make a stand here.
Is "some audit and some fuzzing" a good policy? How much is enough?

> Also BTW of course user space can still override it, but really the defaults
> should be a kernel policy.

Well if userspace sets the policy then I'm not sure we also want
a kernel one ... but if yes I'd like it to be in a central
place so whoever is building the kernel can tweak it easily
and rebuild, without poking at individual drivers.

> There's also the additional problem that one of the goals of confidential
> guest is to just move existing guest virtual images into them without much
> changes. So it's better for such a case if as much as possible of the policy
> is in the kernel.

If it's e.g. the kernel command line then we can set that
when building the kernel right? No need for a dedicated interface
just for this ...

> But that's more a secondary consideration, the first point
> is really the important part.
> 
> 
> -Andi

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Greg Kroah-Hartman]

On Sat, Oct 02, 2021 at 02:40:55PM -0400, Michael S. Tsirkin wrote:
> On Sat, Oct 02, 2021 at 07:20:22AM -0700, Andi Kleen wrote:
> > 
> > On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> > > > On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> > > > > >    Do you have a list of specific drivers and kernel options that you
> > > > > > feel you now "trust"?
> > > > > For TDX it's currently only virtio net/block/console
> > > > > 
> > > > > But we expect this list to grow slightly over time, but not at a high rate
> > > > > (so hopefully <10)
> > > > Well there are already >10 virtio drivers and I think it's reasonable
> > > > that all of these will be used with encrypted guests. The list will
> > > > grow.
> > > What is keeping "all" drivers from being on this list?
> > 
> > It would be too much work to harden them all, and it would be pointless
> > because all these drivers are never legitimately needed in a virtualized
> > environment which only virtualize a very small number of devices.
> > 
> > >   How exactly are
> > > you determining what should, and should not, be allowed?
> > 
> > Everything that has had reasonable effort at hardening can be added. But if
> > someone proposes to add a driver that should trigger additional scrutiny in
> > code review. We should also request them to do some fuzzing.
> 
> Looks like out of tree modules get a free pass then.

That's not good.  As we already know if a module is in or out of the
tree, you all should be banning all out-of-tree modules if you care
about these things.  That should be very easy to do if you care.

> > How would user space know what drivers have been hardened? This is really
> > something that the kernel needs to determine. I don't think we can outsource
> > it to anyone else.
> 
> IIUC userspace is the distro. It can also do more than a binary on/off,
> e.g. it can decide "only virtio", "no out of tree drivers".
> A distro can also ship configs with a specific features
> enabled/disabled. E.g. I can see where some GPU drivers will be
> included by some distros since they are so useful, and excluded
> by others since they are so big and hard to audit.
> I don't see how the kernel can reasonably make a stand here.
> Is "some audit and some fuzzing" a good policy? How much is enough?

Agreed, that is why the policy for this should be in userspace.

> Well if userspace sets the policy then I'm not sure we also want
> a kernel one ... but if yes I'd like it to be in a central
> place so whoever is building the kernel can tweak it easily
> and rebuild, without poking at individual drivers.

And here I thought the requirement was that no one could rebuild their
kernel as it was provided by someone else.

Again, these requirements seem contradicting, but as no one has actually
pointed me at the real list of them, who knows what they are?

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Dan Williams]

On Sat, Oct 2, 2021 at 7:20 AM Andi Kleen <ak@linux.intel.com> wrote:
>
>
> On 10/2/2021 4:14 AM, Greg Kroah-Hartman wrote:
> > On Sat, Oct 02, 2021 at 07:04:28AM -0400, Michael S. Tsirkin wrote:
> >> On Fri, Oct 01, 2021 at 08:49:28AM -0700, Andi Kleen wrote:
> >>>>    Do you have a list of specific drivers and kernel options that you
> >>>> feel you now "trust"?
> >>> For TDX it's currently only virtio net/block/console
> >>>
> >>> But we expect this list to grow slightly over time, but not at a high rate
> >>> (so hopefully <10)
> >> Well there are already >10 virtio drivers and I think it's reasonable
> >> that all of these will be used with encrypted guests. The list will
> >> grow.
> > What is keeping "all" drivers from being on this list?
>
> It would be too much work to harden them all, and it would be pointless
> because all these drivers are never legitimately needed in a virtualized
> environment which only virtualize a very small number of devices.
>
> >   How exactly are
> > you determining what should, and should not, be allowed?
>
> Everything that has had reasonable effort at hardening can be added. But
> if someone proposes to add a driver that should trigger additional
> scrutiny in code review. We should also request them to do some fuzzing.
>
> It's a bit similar to someone trying to add a new syscall interface.
> That also triggers much additional scrutiny for good reasons and people
> start fuzzing it.
>
>
> >    How can
> > drivers move on, or off, of it over time?
>
> Adding something is submitting a patch to the allow list.
>
> I'm not sure the "off" case would happen, unless the driver is
> completely removed, or maybe it has some unfixable security problem. But
> that is all rather unlikely.
>
>
> >
> > And why not just put all of that into userspace and have it pick and
> > choose?  That should be the end-goal here, you don't want to encode
> > policy like this in the kernel, right?
>
> How would user space know what drivers have been hardened? This is
> really something that the kernel needs to determine. I don't think we
> can outsource it to anyone else.

How it is outsourcing by moving that same allow list over the kernel /
user boundary. It can be maintained by the same engineers and get
deployed by something like:

dracut --authorize-device-list=confidential-computing-default $kernel-version

With that distributions can deploy kernel-specific authorizations and
admins can deploy site-specific authorizations. Then the kernel
implementation is minimized to authorize just enough drivers by
default to let userspace take over the policy.

> Also BTW of course user space can still override it, but really the
> defaults should be a kernel policy.

The default is secure, trust nothing but bootstrap devices.

> There's also the additional problem that one of the goals of
> confidential guest is to just move existing guest virtual images into
> them without much changes. So it's better for such a case if as much as
> possible of the policy is in the kernel. But that's more a secondary
> consideration, the first point is really the important part.

The same image can be used on host and guest in this "do it in
userspace" proposal.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Dan Williams]

On Fri, Oct 1, 2021 at 12:03 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 30, 2021 at 12:04:05PM -0700, Kuppuswamy, Sathyanarayanan wrote:
> >
> >
> > On 9/30/21 8:23 AM, Greg Kroah-Hartman wrote:
> > > On Thu, Sep 30, 2021 at 08:18:18AM -0700, Kuppuswamy, Sathyanarayanan wrote:
> > > >
> > > >
> > > > On 9/30/21 6:36 AM, Dan Williams wrote:
> > > > > > And in particular, not all virtio drivers are hardened -
> > > > > > I think at this point blk and scsi drivers have been hardened - so
> > > > > > treating them all the same looks wrong.
> > > > > My understanding was that they have been audited, Sathya?
> > > >
> > > > Yes, AFAIK, it has been audited. Andi also submitted some patches
> > > > related to it. Andi, can you confirm.
> > >
> > > What is the official definition of "audited"?
> >
> >
> > In our case (Confidential Computing platform), the host is an un-trusted
> > entity. So any interaction with host from the drivers will have to be
> > protected against the possible attack from the host. For example, if we
> > are accessing a memory based on index value received from host, we have
> > to make sure it does not lead to out of bound access or when sharing the
> > memory with the host, we need to make sure only the required region is
> > shared with the host and the memory is un-shared after use properly.
>
> You have not defined the term "audited" here at all in any way that can
> be reviewed or verified by anyone from what I can tell.

Agree.

>
> You have only described a new model that you wish the kernel to run in,
> one in which it does not trust the hardware at all.  That is explicitly
> NOT what the kernel has been designed for so far, and if you wish to
> change that, lots of things need to be done outside of simply running
> some fuzzers on a few random drivers.
>
> For one example, how do you ensure that the memory you are reading from
> hasn't been modified by the host between writing to it the last time you
> did?  Do you have a list of specific drivers and kernel options that you
> feel you now "trust"?  If so, how long does that trust last for?  Until
> someonen else modifies that code?  What about modifications to functions
> that your "audited" code touches?  Who is doing this auditing?  How do
> you know the auditing has been done correctly?  Who has reviewed and
> audited the tools that are doing the auditing?  Where is the
> specification that has been agreed on how the auditing must be done?
> And so on...
>
> I feel like there are a lot of different things all being mixed up here
> into one "oh we want this to happen!" type of thread.  Please let's just
> stick to the one request that I had here, which was to move the way that
> busses are allowed to authorize the devices they wish to control into a
> generic way instead of being bus-specific logic.

I want to continue to shave this proposal down, but that last sentence
reads as self-contradictory.

Bear with me, and perhaps it's a lack of imagination on my part, but I
don't see how to get to a globally generic "authorized" sysfs ABI
given that USB and Thunderbolt want to do bus specific actions on
authorization toggle events. Certainly a default generic authorized
attribute can be defined for all the other buses that don't have
legacy here, but Thunderbolt will still require support for '2' as an
authorized value, and USB will still want to base probe decisions on
the authorization state of both the usb_device and the usb_interface.

> Any requests outside of that type of functionality are just that,
> outside the scope of this patchset and should get their own patch series
> and discussion.

A way to shave this further would be to default authorize just enough
devices to do the rest of the authorization from initramfs. That would
seem to cut out 'virtio-net' and 'virtio-storage' from default
authorized list.

The generic authorized attribute would only need to appear when
'dev_default_authorization' is set to false.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Alan Stern]

On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> Bear with me, and perhaps it's a lack of imagination on my part, but I
> don't see how to get to a globally generic "authorized" sysfs ABI
> given that USB and Thunderbolt want to do bus specific actions on
> authorization toggle events. Certainly a default generic authorized
> attribute can be defined for all the other buses that don't have
> legacy here, but Thunderbolt will still require support for '2' as an
> authorized value, and USB will still want to base probe decisions on
> the authorization state of both the usb_device and the usb_interface.

The USB part isn't really accurate (I can't speak for Thunderbolt). 
When a usb_device is deauthorized, the device will be unconfigured, 
deleting all its interfaces and removing the need for any probe 
decisions about them.  In other words, the probe decision for a 
usb_device or usb_interface depends only on the device's/interface's 
own authorization state.

True, the interface binding code does contain a test of the device's 
authorization setting.  That test is redundant and can be removed.

The actions that USB wants to take on authorization toggle events for 
usb_devices are: for authorize, select and install a configuration; 
for deauthorize, unconfigure the device.  Each of these could be 
handled simply enough just by binding/unbinding the device.  (There 
is some special code for handling wireless USB devices, but wireless 
USB is now defunct.)

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Dan Williams]

On Fri, Oct 1, 2021 at 9:47 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> > Bear with me, and perhaps it's a lack of imagination on my part, but I
> > don't see how to get to a globally generic "authorized" sysfs ABI
> > given that USB and Thunderbolt want to do bus specific actions on
> > authorization toggle events. Certainly a default generic authorized
> > attribute can be defined for all the other buses that don't have
> > legacy here, but Thunderbolt will still require support for '2' as an
> > authorized value, and USB will still want to base probe decisions on
> > the authorization state of both the usb_device and the usb_interface.
>
> The USB part isn't really accurate (I can't speak for Thunderbolt).
> When a usb_device is deauthorized, the device will be unconfigured,
> deleting all its interfaces and removing the need for any probe
> decisions about them.  In other words, the probe decision for a
> usb_device or usb_interface depends only on the device's/interface's
> own authorization state.
>
> True, the interface binding code does contain a test of the device's
> authorization setting.  That test is redundant and can be removed.
>
> The actions that USB wants to take on authorization toggle events for
> usb_devices are: for authorize, select and install a configuration;
> for deauthorize, unconfigure the device.  Each of these could be
> handled simply enough just by binding/unbinding the device.  (There
> is some special code for handling wireless USB devices, but wireless
> USB is now defunct.)

Ah, so are you saying that it would be sufficient for USB if the
generic authorized implementation did something like:

dev->authorized = 1;
device_attach(dev);

...for the authorize case, and:

dev->authorize = 0;
device_release_driver(dev);

...for the deauthorize case?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Alan Stern]

On Fri, Oct 01, 2021 at 11:09:52AM -0700, Dan Williams wrote:
> On Fri, Oct 1, 2021 at 9:47 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> > > Bear with me, and perhaps it's a lack of imagination on my part, but I
> > > don't see how to get to a globally generic "authorized" sysfs ABI
> > > given that USB and Thunderbolt want to do bus specific actions on
> > > authorization toggle events. Certainly a default generic authorized
> > > attribute can be defined for all the other buses that don't have
> > > legacy here, but Thunderbolt will still require support for '2' as an
> > > authorized value, and USB will still want to base probe decisions on
> > > the authorization state of both the usb_device and the usb_interface.
> >
> > The USB part isn't really accurate (I can't speak for Thunderbolt).
> > When a usb_device is deauthorized, the device will be unconfigured,
> > deleting all its interfaces and removing the need for any probe
> > decisions about them.  In other words, the probe decision for a
> > usb_device or usb_interface depends only on the device's/interface's
> > own authorization state.
> >
> > True, the interface binding code does contain a test of the device's
> > authorization setting.  That test is redundant and can be removed.
> >
> > The actions that USB wants to take on authorization toggle events for
> > usb_devices are: for authorize, select and install a configuration;
> > for deauthorize, unconfigure the device.  Each of these could be
> > handled simply enough just by binding/unbinding the device.  (There
> > is some special code for handling wireless USB devices, but wireless
> > USB is now defunct.)
> 
> Ah, so are you saying that it would be sufficient for USB if the
> generic authorized implementation did something like:
> 
> dev->authorized = 1;
> device_attach(dev);
> 
> ...for the authorize case, and:
> 
> dev->authorize = 0;
> device_release_driver(dev);
> 
> ...for the deauthorize case?

Yes, I think so.  But I haven't tried making this change to test and 
see what really happens.

Alan Stern
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Dan Williams]

On Fri, Oct 1, 2021 at 12:02 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, Oct 01, 2021 at 11:09:52AM -0700, Dan Williams wrote:
> > On Fri, Oct 1, 2021 at 9:47 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Fri, Oct 01, 2021 at 09:13:54AM -0700, Dan Williams wrote:
> > > > Bear with me, and perhaps it's a lack of imagination on my part, but I
> > > > don't see how to get to a globally generic "authorized" sysfs ABI
> > > > given that USB and Thunderbolt want to do bus specific actions on
> > > > authorization toggle events. Certainly a default generic authorized
> > > > attribute can be defined for all the other buses that don't have
> > > > legacy here, but Thunderbolt will still require support for '2' as an
> > > > authorized value, and USB will still want to base probe decisions on
> > > > the authorization state of both the usb_device and the usb_interface.
> > >
> > > The USB part isn't really accurate (I can't speak for Thunderbolt).
> > > When a usb_device is deauthorized, the device will be unconfigured,
> > > deleting all its interfaces and removing the need for any probe
> > > decisions about them.  In other words, the probe decision for a
> > > usb_device or usb_interface depends only on the device's/interface's
> > > own authorization state.
> > >
> > > True, the interface binding code does contain a test of the device's
> > > authorization setting.  That test is redundant and can be removed.
> > >
> > > The actions that USB wants to take on authorization toggle events for
> > > usb_devices are: for authorize, select and install a configuration;
> > > for deauthorize, unconfigure the device.  Each of these could be
> > > handled simply enough just by binding/unbinding the device.  (There
> > > is some special code for handling wireless USB devices, but wireless
> > > USB is now defunct.)
> >
> > Ah, so are you saying that it would be sufficient for USB if the
> > generic authorized implementation did something like:
> >
> > dev->authorized = 1;
> > device_attach(dev);
> >
> > ...for the authorize case, and:
> >
> > dev->authorize = 0;
> > device_release_driver(dev);
> >
> > ...for the deauthorize case?
>
> Yes, I think so.  But I haven't tried making this change to test and
> see what really happens.

Sounds like a useful path for this effort to explore. Especially as
Greg seems to want the proposed "has_probe_authorization" flag in the
bus_type to disappear and make this all generic. It just seems that
Thunderbolt would need deeper surgery to move what it does in the
authorization toggle path into the probe and remove paths.

Mika, do you see a path for Thunderbolt to align its authorization
paths behind bus ->probe() ->remove() events similar to what USB might
be able to support for a generic authorization path?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Dan Williams]

On Sun, Oct 3, 2021 at 10:16 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> Hi,
>
> On Fri, Oct 01, 2021 at 12:57:18PM -0700, Dan Williams wrote:
> > > > Ah, so are you saying that it would be sufficient for USB if the
> > > > generic authorized implementation did something like:
> > > >
> > > > dev->authorized = 1;
> > > > device_attach(dev);
> > > >
> > > > ...for the authorize case, and:
> > > >
> > > > dev->authorize = 0;
> > > > device_release_driver(dev);
> > > >
> > > > ...for the deauthorize case?
> > >
> > > Yes, I think so.  But I haven't tried making this change to test and
> > > see what really happens.
> >
> > Sounds like a useful path for this effort to explore. Especially as
> > Greg seems to want the proposed "has_probe_authorization" flag in the
> > bus_type to disappear and make this all generic. It just seems that
> > Thunderbolt would need deeper surgery to move what it does in the
> > authorization toggle path into the probe and remove paths.
> >
> > Mika, do you see a path for Thunderbolt to align its authorization
> > paths behind bus ->probe() ->remove() events similar to what USB might
> > be able to support for a generic authorization path?
>
> In Thunderbolt "authorization" actually means whether there is a PCIe
> tunnel to the device or not. There is no driver bind/unbind happening
> when authorization toggles (well on Thunderbolt bus, there can be on PCI
> bus after the tunnel is established) so I'm not entirely sure how we
> could use the bus ->probe() or ->remove for that to be honest.

Greg, per your comment:

"... which was to move the way that busses are allowed to authorize
the devices they wish to control into a generic way instead of being
bus-specific logic."

We have USB and TB that have already diverged on the ABI here. The USB
behavior is more in line with the "probe authorization" concept, while
TB is about tunnel establishment and not cleanly tied to probe
authorization. So while I see a path to a common authorization
implementation for USB and other buses (per the insight from Alan), TB
needs to retain the ability to record the authorization state as an
enum rather than a bool, and emit a uevent on authorization status
change.

So how about something like the following that moves the attribute
into the core, but still calls back to TB and USB to perform their
legacy authorization work. This new authorized attribute only shows up
when devices default to not authorized, i.e. when userspace owns the
allow list past critical-boot built-in drivers, or if the bus (USB /
TB) implements ->authorize().


diff --git a/drivers/base/core.c b/drivers/base/core.c
index e65dd803a453..8f8fbe2637d1 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2414,6 +2414,58 @@ static ssize_t online_store(struct device *dev,
struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(online);

+static ssize_t authorized_show(struct device *dev,
+                              struct device_attribute *attr, char *buf)
+{
+       return sysfs_emit(buf, "%u\n", dev->authorized);
+}
+
+static ssize_t authorized_store(struct device *dev,
+                               struct device_attribute *attr, const char *buf,
+                               size_t count)
+{
+       unsigned int val, save;
+       ssize_t rc;
+
+       rc = kstrtouint(buf, 0, &val);
+       if (rc < 0)
+               return rc;
+
+       /* some buses (Thunderbolt) support authorized values > 1 */
+       if (val > 1 && !dev->bus->authorize)
+               return -EINVAL;
+
+       device_lock(dev);
+       save = dev->authorized;
+       if (save == val) {
+               rc = count;
+               goto err;
+       }
+
+       dev->authorized = val;
+       if (dev->bus->authorize) {
+               /* notify bus about change in authorization state */
+               rc = dev->bus->authorize(dev);
+               if (rc) {
+                       dev->authorized = save;
+                       goto err;
+               }
+       }
+       device_unlock(dev);
+
+       if (dev->authorized) {
+               if (!device_attach(dev))
+                       dev_dbg(dev, "failed to probe after authorize\n");
+       } else
+               device_release_driver(dev);
+       return count;
+
+err:
+       device_unlock(dev);
+       return rc < 0 ? rc : count;
+}
+static DEVICE_ATTR_RW(authorized);
+
 static ssize_t removable_show(struct device *dev, struct
device_attribute *attr,
                              char *buf)
 {
@@ -2616,8 +2668,16 @@ static int device_add_attrs(struct device *dev)
                        goto err_remove_dev_waiting_for_supplier;
        }

+       if (dev_needs_authorization(dev)) {
+               error = device_create_file(dev, &dev_attr_authorized);
+               if (error)
+                       goto err_remove_dev_removable;
+       }
+
        return 0;

+ err_remove_dev_removable:
+       device_remove_file(dev, &dev_attr_removable);
  err_remove_dev_waiting_for_supplier:
        device_remove_file(dev, &dev_attr_waiting_for_supplier);
  err_remove_dev_online:
@@ -2639,6 +2699,7 @@ static void device_remove_attrs(struct device *dev)
        struct class *class = dev->class;
        const struct device_type *type = dev->type;

+       device_remove_file(dev, &dev_attr_authorized);
        device_remove_file(dev, &dev_attr_removable);
        device_remove_file(dev, &dev_attr_waiting_for_supplier);
        device_remove_file(dev, &dev_attr_online);
@@ -2805,6 +2866,8 @@ static void klist_children_put(struct klist_node *n)
        put_device(dev);
 }

+unsigned int dev_default_authorization;
+
 /**
  * device_initialize - init device structure.
  * @dev: device.
diff --git a/include/linux/device.h b/include/linux/device.h
index e270cb740b9e..fbb83e46af9d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -561,6 +561,7 @@ struct device {
        struct dev_iommu        *iommu;

        enum device_removable   removable;
+       unsigned int            authorized;

        bool                    offline_disabled:1;
        bool                    offline:1;
@@ -814,6 +815,19 @@ static inline bool dev_removable_is_valid(struct
device *dev)
        return dev->removable != DEVICE_REMOVABLE_NOT_SUPPORTED;
 }

+extern unsigned int dev_default_authorization;
+
+/*
+ * If the bus has custom authorization, or if devices default to not
+ * authorized, register the 'authorized' attribute for @dev.
+ */
+static inline bool dev_needs_authorization(struct device *dev)
+{
+       if (dev->bus->authorize || dev_default_authorization == 0)
+               return true;
+       return false;
+}
+
 /*
  * High level routines for use by the bus drivers
  */
diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
index 062777a45a74..3202a2b13374 100644
--- a/include/linux/device/bus.h
+++ b/include/linux/device/bus.h
@@ -40,6 +40,11 @@ struct fwnode_handle;
  *             that generate uevents to add the environment variables.
  * @probe:     Called when a new device or driver add to this bus, and callback
  *             the specific driver's probe to initial the matched device.
+ * @authorize: Called after authorized_store() changes the
+ *             authorization state of the device. Do not use for new
+ *             bus implementations, revalidate dev->authorized in
+ *             @probe and @remove to take any bus specific
+ *             authorization actions.
  * @sync_state:        Called to sync device state to software state
after all the
  *             state tracking consumers linked to this device (present at
  *             the time of late_initcall) have successfully bound to a
@@ -90,6 +95,7 @@ struct bus_type {
        int (*match)(struct device *dev, struct device_driver *drv);
        int (*uevent)(struct device *dev, struct kobj_uevent_env *env);
        int (*probe)(struct device *dev);
+       int (*authorize)(struct device *dev);
        void (*sync_state)(struct device *dev);
        void (*remove)(struct device *dev);
        void (*shutdown)(struct device *dev);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Greg Kroah-Hartman]

On Tue, Oct 05, 2021 at 03:33:29PM -0700, Dan Williams wrote:
> On Sun, Oct 3, 2021 at 10:16 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > Hi,
> >
> > On Fri, Oct 01, 2021 at 12:57:18PM -0700, Dan Williams wrote:
> > > > > Ah, so are you saying that it would be sufficient for USB if the
> > > > > generic authorized implementation did something like:
> > > > >
> > > > > dev->authorized = 1;
> > > > > device_attach(dev);
> > > > >
> > > > > ...for the authorize case, and:
> > > > >
> > > > > dev->authorize = 0;
> > > > > device_release_driver(dev);
> > > > >
> > > > > ...for the deauthorize case?
> > > >
> > > > Yes, I think so.  But I haven't tried making this change to test and
> > > > see what really happens.
> > >
> > > Sounds like a useful path for this effort to explore. Especially as
> > > Greg seems to want the proposed "has_probe_authorization" flag in the
> > > bus_type to disappear and make this all generic. It just seems that
> > > Thunderbolt would need deeper surgery to move what it does in the
> > > authorization toggle path into the probe and remove paths.
> > >
> > > Mika, do you see a path for Thunderbolt to align its authorization
> > > paths behind bus ->probe() ->remove() events similar to what USB might
> > > be able to support for a generic authorization path?
> >
> > In Thunderbolt "authorization" actually means whether there is a PCIe
> > tunnel to the device or not. There is no driver bind/unbind happening
> > when authorization toggles (well on Thunderbolt bus, there can be on PCI
> > bus after the tunnel is established) so I'm not entirely sure how we
> > could use the bus ->probe() or ->remove for that to be honest.
> 
> Greg, per your comment:
> 
> "... which was to move the way that busses are allowed to authorize
> the devices they wish to control into a generic way instead of being
> bus-specific logic."
> 
> We have USB and TB that have already diverged on the ABI here. The USB
> behavior is more in line with the "probe authorization" concept, while
> TB is about tunnel establishment and not cleanly tied to probe
> authorization. So while I see a path to a common authorization
> implementation for USB and other buses (per the insight from Alan), TB
> needs to retain the ability to record the authorization state as an
> enum rather than a bool, and emit a uevent on authorization status
> change.
> 
> So how about something like the following that moves the attribute
> into the core, but still calls back to TB and USB to perform their
> legacy authorization work. This new authorized attribute only shows up
> when devices default to not authorized, i.e. when userspace owns the
> allow list past critical-boot built-in drivers, or if the bus (USB /
> TB) implements ->authorize().

At quick glance, this looks better, but it would be good to see someone
test it :)

thanks,

greg k-h
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest

[Andi Kleen]

On 9/30/2021 8:18 AM, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 9/30/21 6:36 AM, Dan Williams wrote:
>>> And in particular, not all virtio drivers are hardened -
>>> I think at this point blk and scsi drivers have been hardened - so
>>> treating them all the same looks wrong.
>> My understanding was that they have been audited, Sathya?
>
> Yes, AFAIK, it has been audited. Andi also submitted some patches
> related to it. Andi, can you confirm.
>
> We also authorize the virtio at PCI ID level. And currently we allow
> console, block and net virtio PCI devices.
>
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_NET) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_BLOCK) },
> { PCI_DEVICE(PCI_VENDOR_ID_REDHAT_QUMRANET, VIRTIO_TRANS_ID_CONSOLE) },
>
The only drivers that are being audited and fuzzed are these three 
virtio drivers (in addition to some other x86 code outside the driver model)

-Andi

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization