Re: [RFC PATCH 1/1] virtio: write back features before verify
Am 30.09.21 um 03:20 schrieb Halil Pasic:
This patch fixes a regression introduced by commit 82e89ea077b9
("virtio-blk: Add validation for block size in config space") and
enables similar checks in verify() on big endian platforms.
The problem with checking multi-byte config fields in the verify
callback, on big endian platforms, and with a possibly transitional
device is the following. The verify() callback is called between
config->get_features() and virtio_finalize_features(). That we have a
device that offered F_VERSION_1 then we have the following options
either the device is transitional, and then it has to present the legacy
interface, i.e. a big endian config space until F_VERSION_1 is
negotiated, or we have a non-transitional device, which makes
F_VERSION_1 mandatory, and only implements the non-legacy interface and
thus presents a little endian config space. Because at this point we
can't know if the device is transitional or non-transitional, we can't
know do we need to byte swap or not.
The virtio spec explicitly states that the driver MAY read config
between reading and writing the features so saying that first accessing
the config before feature negotiation is done is not an option. The
specification ain't clear about setting the features multiple times
before FEATURES_OK, so I guess that should be fine.
I don't consider this patch super clean, but frankly I don't think we
have a ton of options. Another option that may or man not be cleaner,
but is also IMHO much uglier is to figure out whether the device is
transitional by rejecting _F_VERSION_1, then resetting it and proceeding
according tho what we have figured out, hoping that the characteristics
of the device didn't change.
Signed-off-by: Halil Pasic
Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config
space")
Reported-by: mark...@us.ibm.com
To make sure that it lands there, meybe add
cc stable 5.14
---
drivers/virtio/virtio.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 0a5b54034d4b..9dc3cfa17b1c 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
if (device_features & (1ULL << i))
__virtio_set_bit(dev, i);
+ /* Write back features before validate to know endianness */
+ if (device_features & (1ULL << VIRTIO_F_VERSION_1))
+ dev->config->finalize_features(dev);
+
if (drv->validate) {
err = drv->validate(dev);
if (err)
base-commit: 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH][next] drm/virtio: fix potential integer overflow on shift of a int
From: Colin Ian King
The left shift of unsigned int 32 bit integer constant 1 is evaluated
using 32 bit arithmetic and then assigned to a signed 64 bit integer.
In the case where i is 32 or more this can lead to an overflow. Fix
this by shifting the value 1ULL instead.
Addresses-Coverity: ("Uninitentional integer overflow")
Fixes: 8d6b006e1f51 ("drm/virtio: implement context init: handle
VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK")
Signed-off-by: Colin Ian King
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 5618a1d5879c..b3b0557d72cf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -819,7 +819,7 @@ static int virtio_gpu_context_init_ioctl(struct drm_device
*dev,
if (vfpriv->ring_idx_mask) {
valid_ring_mask = 0;
for (i = 0; i < vfpriv->num_rings; i++)
- valid_ring_mask |= 1 << i;
+ valid_ring_mask |= 1ULL << i;
if (~valid_ring_mask & vfpriv->ring_idx_mask) {
ret = -EINVAL;
--
2.32.0
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH RFC v1 01/11] uapi/virtio-iommu: Add page request grp-id and flags information
On Thu, Sep 30, 2021 at 10:26:35AM +0530, Vivek Kumar Gautam wrote: > > I'm not sure why we made it 32-bit in Linux UAPI, it's a little wasteful. > > PCIe PRGI is 9-bits and SMMU STAG is 16-bits. Since the scope of the grpid > > is the endpoint, 16-bit means 64k in-flight faults per endpoint, which > > seems more than enough. > > Right, I will update this to 16-bits field. It won't be okay to update the > iommu uAPI now, right? Since there is no UAPI transport for the fault request/response at the moment, it should be possible to update it. Thanks, Jean ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] virtio: write back features before verify
On Thu, Sep 30 2021, Halil Pasic wrote:
> This patch fixes a regression introduced by commit 82e89ea077b9
> ("virtio-blk: Add validation for block size in config space") and
> enables similar checks in verify() on big endian platforms.
>
> The problem with checking multi-byte config fields in the verify
> callback, on big endian platforms, and with a possibly transitional
> device is the following. The verify() callback is called between
> config->get_features() and virtio_finalize_features(). That we have a
> device that offered F_VERSION_1 then we have the following options
> either the device is transitional, and then it has to present the legacy
> interface, i.e. a big endian config space until F_VERSION_1 is
> negotiated, or we have a non-transitional device, which makes
> F_VERSION_1 mandatory, and only implements the non-legacy interface and
> thus presents a little endian config space. Because at this point we
> can't know if the device is transitional or non-transitional, we can't
> know do we need to byte swap or not.
>
> The virtio spec explicitly states that the driver MAY read config
> between reading and writing the features so saying that first accessing
> the config before feature negotiation is done is not an option. The
> specification ain't clear about setting the features multiple times
> before FEATURES_OK, so I guess that should be fine.
>
> I don't consider this patch super clean, but frankly I don't think we
> have a ton of options. Another option that may or man not be cleaner,
> but is also IMHO much uglier is to figure out whether the device is
> transitional by rejecting _F_VERSION_1, then resetting it and proceeding
> according tho what we have figured out, hoping that the characteristics
> of the device didn't change.
>
> Signed-off-by: Halil Pasic
> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config
> space")
> Reported-by: mark...@us.ibm.com
> ---
> drivers/virtio/virtio.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 0a5b54034d4b..9dc3cfa17b1c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
> if (device_features & (1ULL << i))
> __virtio_set_bit(dev, i);
>
> + /* Write back features before validate to know endianness */
> + if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> + dev->config->finalize_features(dev);
This really looks like a mess :(
We end up calling ->finalize_features twice: once before ->validate, and
once after, that time with the complete song and dance. The first time,
we operate on one feature set; after validation, we operate on another,
and there might be interdependencies between the two (like a that a bit
is cleared because of another bit, which would not happen if validate
had a chance to clear that bit before).
I'm not sure whether that is even a problem in the spec: while the
driver may read the config before finally accepting features, it does
not really make sense to do so before a feature bit as basic as
VERSION_1 which determines the endianness has been negotiated. For
VERSION_1, we can probably go ahead and just assume that we will accept
it if offered, but what about other (future) bits?
> +
> if (drv->validate) {
> err = drv->validate(dev);
> if (err)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH][next] drm/virtio: fix another potential integer overflow on shift of a int
From: Colin Ian King
The left shift of unsigned int 32 bit integer constant 1 is evaluated
using 32 bit arithmetic and then assigned to a signed 64 bit integer.
In the case where value is 32 or more this can lead to an overflow
(value can be in range 0..MAX_CAPSET_ID (63). Fix this by shifting
the value 1ULL instead.
Addresses-Coverity: ("Uninitentional integer overflow")
Fixes: 4fb530e5caf7 ("drm/virtio: implement context init: support init ioctl")
Signed-off-by: Colin Ian King
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index b3b0557d72cf..0007e423d885 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -774,7 +774,7 @@ static int virtio_gpu_context_init_ioctl(struct drm_device
*dev,
goto out_unlock;
}
- if ((vgdev->capset_id_mask & (1 << value)) == 0) {
+ if ((vgdev->capset_id_mask & (1ULL << value)) == 0) {
ret = -EINVAL;
goto out_unlock;
}
--
2.32.0
___
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
On Thu, Sep 30, 2021 at 6:59 AM Dan Williams wrote:
>
> On Wed, Sep 29, 2021 at 7:39 PM Kuppuswamy, Sathyanarayanan
> 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(>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(>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 4/6] virtio: Initialize authorized attribute for confidential guest
On Thu, Sep 30, 2021 at 4:03 AM Michael S. Tsirkin 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 > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > 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/yquaj78y8j1um...@kroah.com/ [2]: https://lore.kernel.org/all/yqzf%2futgrjfbz...@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
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 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 > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > 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/yquaj78y8j1um...@kroah.com/ > [2]: https://lore.kernel.org/all/yqzf%2futgrjfbz...@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 2/6] driver core: Add common support to skip probe for un-authorized devices
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 > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > > > > > So what e.g. the PCI patch > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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
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
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
> Signed-off-by: Kuppuswamy Sathyanarayanan
>
So what e.g. the PCI patch
https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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: [RFC PATCH 1/1] virtio: write back features before verify
On Thu, 30 Sep 2021 11:28:23 +0200
Cornelia Huck wrote:
> On Thu, Sep 30 2021, Halil Pasic wrote:
>
> > This patch fixes a regression introduced by commit 82e89ea077b9
> > ("virtio-blk: Add validation for block size in config space") and
> > enables similar checks in verify() on big endian platforms.
> >
> > The problem with checking multi-byte config fields in the verify
> > callback, on big endian platforms, and with a possibly transitional
> > device is the following. The verify() callback is called between
> > config->get_features() and virtio_finalize_features(). That we have a
> > device that offered F_VERSION_1 then we have the following options
> > either the device is transitional, and then it has to present the legacy
> > interface, i.e. a big endian config space until F_VERSION_1 is
> > negotiated, or we have a non-transitional device, which makes
> > F_VERSION_1 mandatory, and only implements the non-legacy interface and
> > thus presents a little endian config space. Because at this point we
> > can't know if the device is transitional or non-transitional, we can't
> > know do we need to byte swap or not.
> >
> > The virtio spec explicitly states that the driver MAY read config
> > between reading and writing the features so saying that first accessing
> > the config before feature negotiation is done is not an option. The
> > specification ain't clear about setting the features multiple times
> > before FEATURES_OK, so I guess that should be fine.
> >
> > I don't consider this patch super clean, but frankly I don't think we
> > have a ton of options. Another option that may or man not be cleaner,
> > but is also IMHO much uglier is to figure out whether the device is
> > transitional by rejecting _F_VERSION_1, then resetting it and proceeding
> > according tho what we have figured out, hoping that the characteristics
> > of the device didn't change.
> >
> > Signed-off-by: Halil Pasic
> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config
> > space")
> > Reported-by: mark...@us.ibm.com
> > ---
> > drivers/virtio/virtio.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 0a5b54034d4b..9dc3cfa17b1c 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
> > if (device_features & (1ULL << i))
> > __virtio_set_bit(dev, i);
> >
> > + /* Write back features before validate to know endianness */
> > + if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> > + dev->config->finalize_features(dev);
>
> This really looks like a mess :(
>
> We end up calling ->finalize_features twice: once before ->validate, and
> once after, that time with the complete song and dance. The first time,
> we operate on one feature set; after validation, we operate on another,
> and there might be interdependencies between the two (like a that a bit
> is cleared because of another bit, which would not happen if validate
> had a chance to clear that bit before).
Basically the second set is a subset of the first set.
>
> I'm not sure whether that is even a problem in the spec: while the
> driver may read the config before finally accepting features
I'm not sure I'm following you. Let me please qoute the specification:
"""
4. Read device feature bits, and write the subset of feature bits
understood by the OS and driver to the device. During this step the driver MAY
read (but MUST NOT write) the device-specific configuration fields to check
that it can support the device before accepting it.
5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature bits
after this step.
"""
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-930001
> , it does
> not really make sense to do so before a feature bit as basic as
> VERSION_1 which determines the endianness has been negotiated.
Are you suggesting that ->verify() should be after
virtio_finalize_features()? Wouldn't
that mean that verify() can't reject feature bits? But that is the whole
point of commit 82e89ea077b9 ("virtio-blk: Add validation for block size
in config space"). Do you think that the commit in question is
conceptually flawed? My understanding of the verify is, that it is supposed
to fence features and feature bits we can't support, e.g. because of
config space things, but I may be wrong.
The trouble is, feature bits are not negotiated one by one, but basically all
at once. I suppose, I did the next best thing to first negotiating VERSION_1.
> For
> VERSION_1, we can probably go ahead and just assume that we will accept
> it if offered, but what about other (future) bits?
I don't quite understand.
Anyway, how do you think we should solve this problem?
Regards,
Halil
>
> > +
> > if (drv->validate) {
> > err = drv->validate(dev);
> > if
Re: [PATCH v2 4/6] virtio: Initialize authorized attribute for confidential guest
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 > Signed-off-by: Kuppuswamy Sathyanarayanan > 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 > #include > #include > +#include > +#include > #include > > /* 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 2/6] driver core: Add common support to skip probe for un-authorized devices
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 > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > So what e.g. the PCI patch > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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
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 > > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > > > > > > > > > > So what e.g. the PCI patch > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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
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 > > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > > > > > > > > > > So what e.g. the PCI patch > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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: [RFC PATCH 1/1] virtio: write back features before verify
On Thu, Sep 30 2021, "Michael S. Tsirkin" wrote:
> On Thu, Sep 30, 2021 at 03:20:49AM +0200, Halil Pasic wrote:
>> This patch fixes a regression introduced by commit 82e89ea077b9
>> ("virtio-blk: Add validation for block size in config space") and
>> enables similar checks in verify() on big endian platforms.
>>
>> The problem with checking multi-byte config fields in the verify
>> callback, on big endian platforms, and with a possibly transitional
>> device is the following. The verify() callback is called between
>> config->get_features() and virtio_finalize_features(). That we have a
>> device that offered F_VERSION_1 then we have the following options
>> either the device is transitional, and then it has to present the legacy
>> interface, i.e. a big endian config space until F_VERSION_1 is
>> negotiated, or we have a non-transitional device, which makes
>> F_VERSION_1 mandatory, and only implements the non-legacy interface and
>> thus presents a little endian config space. Because at this point we
>> can't know if the device is transitional or non-transitional, we can't
>> know do we need to byte swap or not.
>
> Hmm which transport does this refer to?
> Distinguishing between legacy and modern drivers is transport
> specific. PCI presents
> legacy and modern at separate addresses so distinguishing
> between these two should be no trouble.
Hm, what about transitional devices?
> Channel i/o has versioning so same thing?
It can turn off VERSION_1, but not legacy. (I had hacked up a patchset
to potentially disable legacy some time ago, but did not have any
resources to follow up on this.)
>
>> The virtio spec explicitly states that the driver MAY read config
>> between reading and writing the features so saying that first accessing
>> the config before feature negotiation is done is not an option. The
>> specification ain't clear about setting the features multiple times
>> before FEATURES_OK, so I guess that should be fine.
>>
>> I don't consider this patch super clean, but frankly I don't think we
>> have a ton of options. Another option that may or man not be cleaner,
>> but is also IMHO much uglier is to figure out whether the device is
>> transitional by rejecting _F_VERSION_1, then resetting it and proceeding
>> according tho what we have figured out, hoping that the characteristics
>> of the device didn't change.
>
> I am confused here. So is the problem at the device or at the driver level?
> I suspect it's actually the host that has the issue, not
> the guest?
>From my perspective the problem is that the version of the device
remains in limbo as long as the features have not yet been finalized,
which means that the endianness of the config space remains in limbo as
well. Both device and driver might come to different conclusions.
>
>
>> Signed-off-by: Halil Pasic
>> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config
>> space")
>> Reported-by: mark...@us.ibm.com
>> ---
>> drivers/virtio/virtio.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index 0a5b54034d4b..9dc3cfa17b1c 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
>> if (device_features & (1ULL << i))
>> __virtio_set_bit(dev, i);
>>
>> +/* Write back features before validate to know endianness */
>> +if (device_features & (1ULL << VIRTIO_F_VERSION_1))
>> +dev->config->finalize_features(dev);
>> +
>> if (drv->validate) {
>> err = drv->validate(dev);
>> if (err)
>>
>> base-commit: 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
>> --
>> 2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-blk: set NUMA affinity for a tagset
On Wed, Sep 29, 2021 at 06:07:52PM +0300, Max Gurtovoy wrote: > > On 9/28/2021 9:47 AM, Stefan Hajnoczi wrote: > > On Mon, Sep 27, 2021 at 08:39:30PM +0300, Max Gurtovoy wrote: > > > On 9/27/2021 11:09 AM, Stefan Hajnoczi wrote: > > > > On Sun, Sep 26, 2021 at 05:55:18PM +0300, Max Gurtovoy wrote: > > > > > To optimize performance, set the affinity of the block device tagset > > > > > according to the virtio device affinity. > > > > > > > > > > Signed-off-by: Max Gurtovoy > > > > > --- > > > > >drivers/block/virtio_blk.c | 2 +- > > > > >1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c > > > > > index 9b3bd083b411..1c68c3e0ebf9 100644 > > > > > --- a/drivers/block/virtio_blk.c > > > > > +++ b/drivers/block/virtio_blk.c > > > > > @@ -774,7 +774,7 @@ static int virtblk_probe(struct virtio_device > > > > > *vdev) > > > > > memset(>tag_set, 0, sizeof(vblk->tag_set)); > > > > > vblk->tag_set.ops = _mq_ops; > > > > > vblk->tag_set.queue_depth = queue_depth; > > > > > - vblk->tag_set.numa_node = NUMA_NO_NODE; > > > > > + vblk->tag_set.numa_node = virtio_dev_to_node(vdev); > > > > > vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE; > > > > > vblk->tag_set.cmd_size = > > > > > sizeof(struct virtblk_req) + > > > > I implemented NUMA affinity in the past and could not demonstrate a > > > > performance improvement: > > > > https://lists.linuxfoundation.org/pipermail/virtualization/2020-June/048248.html > > > > > > > > The pathological case is when a guest with vNUMA has the virtio-blk-pci > > > > device on the "wrong" host NUMA node. Then memory accesses should cross > > > > NUMA nodes. Still, it didn't seem to matter. > > > I think the reason you didn't see any improvement is since you didn't use > > > the right device for the node query. See my patch 1/2. > > That doesn't seem to be the case. Please see > > drivers/base/core.c:device_add(): > > > >/* use parent numa_node */ > >if (parent && (dev_to_node(dev) == NUMA_NO_NODE)) > >set_dev_node(dev, dev_to_node(parent)); > > > > IMO it's cleaner to use dev_to_node(>dev) than to directly access > > the parent. > > > > Have I missed something? > > but dev_to_node(dev) is 0 IMO. > > who set it to NUMA_NO_NODE ? drivers/virtio/virtio.c:register_virtio_device(): device_initialize(>dev); drivers/base/core.c:device_initialize(): set_dev_node(dev, -1); Stefan signature.asc Description: PGP signature ___ 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
On Wed, Sep 29, 2021 at 06:55:12PM -0700, Dan Williams wrote: > On Wed, Sep 29, 2021 at 6:43 PM Alan Stern 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_gyt2nkzaogdrqj9tks...@mail.gmail.com/ > > > > > > Reviewed-by: Dan Williams > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > 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: [RFC PATCH 1/1] virtio: write back features before verify
On Thu, Sep 30, 2021 at 03:20:49AM +0200, Halil Pasic wrote:
> This patch fixes a regression introduced by commit 82e89ea077b9
> ("virtio-blk: Add validation for block size in config space") and
> enables similar checks in verify() on big endian platforms.
>
> The problem with checking multi-byte config fields in the verify
> callback, on big endian platforms, and with a possibly transitional
> device is the following. The verify() callback is called between
> config->get_features() and virtio_finalize_features(). That we have a
> device that offered F_VERSION_1 then we have the following options
> either the device is transitional, and then it has to present the legacy
> interface, i.e. a big endian config space until F_VERSION_1 is
> negotiated, or we have a non-transitional device, which makes
> F_VERSION_1 mandatory, and only implements the non-legacy interface and
> thus presents a little endian config space. Because at this point we
> can't know if the device is transitional or non-transitional, we can't
> know do we need to byte swap or not.
Hmm which transport does this refer to?
Distinguishing between legacy and modern drivers is transport
specific. PCI presents
legacy and modern at separate addresses so distinguishing
between these two should be no trouble.
Channel i/o has versioning so same thing?
> The virtio spec explicitly states that the driver MAY read config
> between reading and writing the features so saying that first accessing
> the config before feature negotiation is done is not an option. The
> specification ain't clear about setting the features multiple times
> before FEATURES_OK, so I guess that should be fine.
>
> I don't consider this patch super clean, but frankly I don't think we
> have a ton of options. Another option that may or man not be cleaner,
> but is also IMHO much uglier is to figure out whether the device is
> transitional by rejecting _F_VERSION_1, then resetting it and proceeding
> according tho what we have figured out, hoping that the characteristics
> of the device didn't change.
I am confused here. So is the problem at the device or at the driver level?
I suspect it's actually the host that has the issue, not
the guest?
> Signed-off-by: Halil Pasic
> Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config
> space")
> Reported-by: mark...@us.ibm.com
> ---
> drivers/virtio/virtio.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 0a5b54034d4b..9dc3cfa17b1c 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
> if (device_features & (1ULL << i))
> __virtio_set_bit(dev, i);
>
> + /* Write back features before validate to know endianness */
> + if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> + dev->config->finalize_features(dev);
> +
> if (drv->validate) {
> err = drv->validate(dev);
> if (err)
>
> base-commit: 02d5e016800d082058b3d3b7c3ede136cdc6ddcb
> --
> 2.25.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 1/1] virtio: write back features before verify
On Thu, Sep 30 2021, Halil Pasic wrote:
> On Thu, 30 Sep 2021 11:28:23 +0200
> Cornelia Huck wrote:
>
>> On Thu, Sep 30 2021, Halil Pasic wrote:
>>
>> > This patch fixes a regression introduced by commit 82e89ea077b9
>> > ("virtio-blk: Add validation for block size in config space") and
>> > enables similar checks in verify() on big endian platforms.
>> >
>> > The problem with checking multi-byte config fields in the verify
>> > callback, on big endian platforms, and with a possibly transitional
>> > device is the following. The verify() callback is called between
>> > config->get_features() and virtio_finalize_features(). That we have a
>> > device that offered F_VERSION_1 then we have the following options
>> > either the device is transitional, and then it has to present the legacy
>> > interface, i.e. a big endian config space until F_VERSION_1 is
>> > negotiated, or we have a non-transitional device, which makes
>> > F_VERSION_1 mandatory, and only implements the non-legacy interface and
>> > thus presents a little endian config space. Because at this point we
>> > can't know if the device is transitional or non-transitional, we can't
>> > know do we need to byte swap or not.
>> >
>> > The virtio spec explicitly states that the driver MAY read config
>> > between reading and writing the features so saying that first accessing
>> > the config before feature negotiation is done is not an option. The
>> > specification ain't clear about setting the features multiple times
>> > before FEATURES_OK, so I guess that should be fine.
>> >
>> > I don't consider this patch super clean, but frankly I don't think we
>> > have a ton of options. Another option that may or man not be cleaner,
>> > but is also IMHO much uglier is to figure out whether the device is
>> > transitional by rejecting _F_VERSION_1, then resetting it and proceeding
>> > according tho what we have figured out, hoping that the characteristics
>> > of the device didn't change.
>> >
>> > Signed-off-by: Halil Pasic
>> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config
>> > space")
>> > Reported-by: mark...@us.ibm.com
>> > ---
>> > drivers/virtio/virtio.c | 4
>> > 1 file changed, 4 insertions(+)
>> >
>> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> > index 0a5b54034d4b..9dc3cfa17b1c 100644
>> > --- a/drivers/virtio/virtio.c
>> > +++ b/drivers/virtio/virtio.c
>> > @@ -249,6 +249,10 @@ static int virtio_dev_probe(struct device *_d)
>> >if (device_features & (1ULL << i))
>> >__virtio_set_bit(dev, i);
>> >
>> > + /* Write back features before validate to know endianness */
>> > + if (device_features & (1ULL << VIRTIO_F_VERSION_1))
>> > + dev->config->finalize_features(dev);
>>
>> This really looks like a mess :(
>>
>> We end up calling ->finalize_features twice: once before ->validate, and
>> once after, that time with the complete song and dance. The first time,
>> we operate on one feature set; after validation, we operate on another,
>> and there might be interdependencies between the two (like a that a bit
>> is cleared because of another bit, which would not happen if validate
>> had a chance to clear that bit before).
>
> Basically the second set is a subset of the first set.
I don't think that's clear.
>
>>
>> I'm not sure whether that is even a problem in the spec: while the
>> driver may read the config before finally accepting features
>
> I'm not sure I'm following you. Let me please qoute the specification:
> """
> 4. Read device feature bits, and write the subset of feature bits
> understood by the OS and driver to the device. During this step the driver
> MAY read (but MUST NOT write) the device-specific configuration fields to
> check that it can support the device before accepting it.
> 5. Set the FEATURES_OK status bit. The driver MUST NOT accept new feature
> bits after this step.
> """
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-930001
Yes, exactly, it MAY read before accepting features. How does the device
know whether the config space is little-endian or not?
>
>> , it does
>> not really make sense to do so before a feature bit as basic as
>> VERSION_1 which determines the endianness has been negotiated.
>
> Are you suggesting that ->verify() should be after
> virtio_finalize_features()?
No, that would defeat the entire purpose of verify. After
virtio_finalize_features(), we are done with feature negotiation.
> Wouldn't
> that mean that verify() can't reject feature bits? But that is the whole
> point of commit 82e89ea077b9 ("virtio-blk: Add validation for block size
> in config space"). Do you think that the commit in question is
> conceptually flawed? My understanding of the verify is, that it is supposed
> to fence features and feature bits we can't support, e.g. because of
> config space things, but I may be wrong.
No, that commit is not really flawed on its own, I
[PATCH] x86/paravirt: use %rip-relative addressing in hook calls
While using a plain (constant) address works, its use needlessly invokes a SIB addressing mode, making every call site one byte larger than necessary. Instead of using an "i" constraint with address-of operator and a 'c' operand modifier, simply use an ordinary "m" constraint, which the 64-bit compiler will translate to %rip-relative addressing. This way we also tell the compiler the truth about operand usage - the memory location gets actually read, after all. 32-bit code generation is unaffected by the change. Signed-off-by: Jan Beulich --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -278,7 +278,7 @@ extern void (*paravirt_iret)(void); #define paravirt_type(op) \ [paravirt_typenum] "i" (PARAVIRT_PATCH(op)),\ - [paravirt_opptr] "i" (&(pv_ops.op)) + [paravirt_opptr] "m" (pv_ops.op) #define paravirt_clobber(clobber) \ [paravirt_clobber] "i" (clobber) @@ -315,7 +315,7 @@ int paravirt_disable_iospace(void); */ #define PARAVIRT_CALL \ ANNOTATE_RETPOLINE_SAFE \ - "call *%c[paravirt_opptr];" + "call *%[paravirt_opptr];" /* * These macros are intended to wrap calls through one of the paravirt ___ 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
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 > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > > > > > So what e.g. the PCI patch > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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
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 4/6] virtio: Initialize authorized attribute for confidential guest
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
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 2/6] driver core: Add common support to skip probe for un-authorized devices
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 > > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > > > > > > > > > > > > > > > > > > > > So what e.g. the PCI patch > > > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core
On Thu, Sep 30, 2021 at 8:00 AM Alan Stern 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 > > 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_gyt2nkzaogdrqj9tks...@mail.gmail.com/ > > > > > > > > Reviewed-by: Dan Williams > > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > > > > 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
On Thu, Sep 30, 2021 at 4:20 AM Yehezkel Bernat wrote: > > On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan > 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 2/6] driver core: Add common support to skip probe for un-authorized devices
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
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
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 > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan > > > > > > > > > > > > > > > > > > > > > So what e.g. the PCI patch > > > > https://lore.kernel.org/all/CACK8Z6E8pjVeC934oFgr=vb3pulx_gyt2nkzaogdrqj9tks...@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
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
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
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
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 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core
On Thu, Sep 30, 2021 at 12:50 PM Kuppuswamy, Sathyanarayanan 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 1/6] driver core: Move the "authorized" attribute from USB/Thunderbolt to core
On Thu, Sep 30, 2021 at 11:25 AM Yehezkel Bernat wrote: > > On Thu, Sep 30, 2021 at 6:28 PM Dan Williams wrote: > > > > On Thu, Sep 30, 2021 at 4:20 AM Yehezkel Bernat > > wrote: > > > > > > On Thu, Sep 30, 2021 at 4:05 AM Kuppuswamy Sathyanarayanan > > > 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 4/6] virtio: Initialize authorized attribute for confidential guest
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
Re: [PATCH v2 2/6] driver core: Add common support to skip probe for un-authorized devices
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 2/6] driver core: Add common support to skip probe for un-authorized devices
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 4/6] virtio: Initialize authorized attribute for confidential guest
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 2/6] driver core: Add common support to skip probe for un-authorized devices
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
On Thu, Sep 30, 2021 at 1:44 PM Alan Stern 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
On Thu, Sep 30, 2021 at 01:52:59PM -0700, Dan Williams wrote: > On Thu, Sep 30, 2021 at 1:44 PM Alan Stern 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
On Thu, Sep 30, 2021 at 6:41 PM Alan Stern 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 > > 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+layt7yz...@kroah.com/ [2]: https://lore.kernel.org/all/yqzdqm6foezm6...@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
