| Message ID | 20211115020552.2378167-3-baolu.lu@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Fix BUG_ON in vfio_iommu_group_notifier() | expand |
On Mon, Nov 15, 2021 at 10:05:43AM +0800, Lu Baolu wrote: > This extends really_probe() to allow checking for dma ownership conflict > during the driver binding process. By default, the DMA_OWNER_KERNEL is > claimed for the bound driver before calling its .probe() callback. If this > operation fails (e.g. the iommu group of the target device already has the > DMA_OWNER_USER set), the binding process is aborted to avoid breaking the > security contract for devices in the iommu group. > > Without this change, the vfio driver has to listen to a bus BOUND_DRIVER > event and then BUG_ON() in case of dma ownership conflict. This leads to > bad user experience since careless driver binding operation may crash the > system if the admin overlooks the group restriction. > > Aside from bad design, this leads to a security problem as a root user, > even with lockdown=integrity, can force the kernel to BUG. > > Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto > claim in the binding process. Examples include kernel drivers (pci_stub, > PCI bridge drivers, etc.) which don't trigger DMA at all thus can be safely > exempted in DMA ownership check and userspace framework drivers (vfio/vdpa > etc.) which need to manually claim DMA_OWNER_USER when assigning a device > to userspace. > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Link: https://lore.kernel.org/linux-iommu/20210922123931.GI327412@nvidia.com/ > Link: https://lore.kernel.org/linux-iommu/20210928115751.GK964074@nvidia.com/ > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> > --- > include/linux/device/driver.h | 7 ++++++- > drivers/base/dd.c | 12 ++++++++++++ > 2 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h > index a498ebcf4993..25d39c64c4d9 100644 > --- a/include/linux/device/driver.h > +++ b/include/linux/device/driver.h > @@ -54,6 +54,10 @@ enum probe_type { > * @owner: The module owner. > * @mod_name: Used for built-in modules. > * @suppress_bind_attrs: Disables bind/unbind via sysfs. > + * @suppress_auto_claim_dma_owner: Disable auto claiming of kernel DMA owner. > + * Drivers which don't require DMA or want to manually claim the > + * owner type (e.g. userspace driver frameworks) could set this > + * flag. > * @probe_type: Type of the probe (synchronous or asynchronous) to use. > * @of_match_table: The open firmware table. > * @acpi_match_table: The ACPI match table. > @@ -99,7 +103,8 @@ struct device_driver { > struct module *owner; > const char *mod_name; /* used for built-in modules */ > > - bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ > + bool suppress_bind_attrs:1; /* disables bind/unbind via sysfs */ > + bool suppress_auto_claim_dma_owner:1; Can a bool be a bitfield? Is that valid C? And why is that even needed? > enum probe_type probe_type; > > const struct of_device_id *of_match_table; > diff --git a/drivers/base/dd.c b/drivers/base/dd.c > index 68ea1f949daa..ab3333351f19 100644 > --- a/drivers/base/dd.c > +++ b/drivers/base/dd.c > @@ -28,6 +28,7 @@ > #include <linux/pm_runtime.h> > #include <linux/pinctrl/devinfo.h> > #include <linux/slab.h> > +#include <linux/iommu.h> > > #include "base.h" > #include "power/power.h" > @@ -566,6 +567,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) > goto done; > } > > + if (!drv->suppress_auto_claim_dma_owner) { > + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, NULL); > + if (ret) > + return ret; > + } > + This feels wrong to be doing it in the driver core, why doesn't the bus that cares about this handle it instead? You just caused all drivers in the kernel today to set and release this ownership, as none set this flag. Shouldn't it be the other way around? And again, why not in the bus that cares? You only have problems with 1 driver out of thousands, this feels wrong to abuse the driver core this way for just that one. thanks, greg k-h
On Mon, Nov 15, 2021 at 10:05:43AM +0800, Lu Baolu wrote: > @@ -566,6 +567,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) > goto done; > } > > + if (!drv->suppress_auto_claim_dma_owner) { > + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, NULL); > + if (ret) > + return ret; > + } I'd expect this to go into iommu_setup_dma_ops (and its arm and s390 equivalents), as that is what claims an IOMMU for in-kernel usage
On Mon, Nov 15, 2021 at 07:59:10AM +0100, Greg Kroah-Hartman wrote: > This feels wrong to be doing it in the driver core, why doesn't the bus > that cares about this handle it instead? > > You just caused all drivers in the kernel today to set and release this > ownership, as none set this flag. Shouldn't it be the other way around? > > And again, why not in the bus that cares? > > You only have problems with 1 driver out of thousands, this feels wrong > to abuse the driver core this way for just that one. This isn't really a bus thingy, but related to the IOMMU subsystem. That being said as pointed out in my previous reply I'd expect this to go along with other IOMMU setup.
On Mon, Nov 15, 2021 at 05:19:02AM -0800, Christoph Hellwig wrote: > On Mon, Nov 15, 2021 at 10:05:43AM +0800, Lu Baolu wrote: > > @@ -566,6 +567,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) > > goto done; > > } > > > > + if (!drv->suppress_auto_claim_dma_owner) { > > + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, NULL); > > + if (ret) > > + return ret; > > + } > > I'd expect this to go into iommu_setup_dma_ops (and its arm and s390 > equivalents), as that is what claims an IOMMU for in-kernel usage If iommu_device_set_dma_owner(dev_a) fails changes dynamically depending on what iommu_device_set_dma_owner(dev_b, DMA_OWNER_USER) have been done. The whole point here is that doing a iommu_device_set_dma_owner(dev_b, DMA_OWNER_USER) needs to revoke kernel usage from a whole bunch of other devices in the same group. revoking kernel usage means it needs to ensure that no driver is bound and prevent future drivers from being bound. iommu_setup_dma_ops() is something done once early on in boot, not at every driver probe, so I don't see how it can help?? Jason
On Mon, Nov 15, 2021 at 07:59:10AM +0100, Greg Kroah-Hartman wrote: > > @@ -566,6 +567,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) > > goto done; > > } > > > > + if (!drv->suppress_auto_claim_dma_owner) { > > + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, NULL); > > + if (ret) > > + return ret; > > + } > > + > > This feels wrong to be doing it in the driver core, why doesn't the bus > that cares about this handle it instead? As Christoph said, it is not related to the bus. To elaborate any bus_type that has iommu_ops != NULL needs this check, and it must be done on an individual struct device as the result is sensitive to the iommu_group member of each struct device. > You just caused all drivers in the kernel today to set and release this > ownership, as none set this flag. Shouldn't it be the other way around? No - the whole point is to cause every driver to do this test. iommu_device_set_dma_owner() can fail for any device, if it does then a kernel driver must not be probed. Probing a kernel driver when iommu_device_set_dma_owner() fails will break kernel integrity due to HW limitations. The drv->suppress_auto_claim_dma_owner disables this restriction because three drivers will deal with DMA ownership on their own. > You only have problems with 1 driver out of thousands, this feels wrong > to abuse the driver core this way for just that one. I think you have it backwards. Few drivers out of thousands can take an action that impacts the security of a thousand other drivers. The key thing is that device A can have a driver with suppress_auto_claim_dma_owner=1 and call iommu_device_set_dma_owner(DMA_OWNER_USER) which will then cause another device B to be unsable in the kernel. Device B, with a normal driver, must be prevented from having a kernel driver because of what the special driver on device A did. This behavior is a IOMMU HW limitation that cannot be avoided. The restrictions have always been in the kernel, they were just enforced with a BUG_ON at probe via a bus_notifier instead of a clean failure. So, I don't know how to block probing of the thousands of drivers without adding a test during probing, do you have an different idea? Jason
On 2021-11-15 13:24, Jason Gunthorpe via iommu wrote: > On Mon, Nov 15, 2021 at 05:19:02AM -0800, Christoph Hellwig wrote: >> On Mon, Nov 15, 2021 at 10:05:43AM +0800, Lu Baolu wrote: >>> @@ -566,6 +567,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) >>> goto done; >>> } >>> >>> + if (!drv->suppress_auto_claim_dma_owner) { >>> + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, NULL); >>> + if (ret) >>> + return ret; >>> + } >> >> I'd expect this to go into iommu_setup_dma_ops (and its arm and s390 >> equivalents), as that is what claims an IOMMU for in-kernel usage > > If iommu_device_set_dma_owner(dev_a) fails changes dynamically > depending on what iommu_device_set_dma_owner(dev_b, DMA_OWNER_USER) > have been done. > > The whole point here is that doing a > iommu_device_set_dma_owner(dev_b, DMA_OWNER_USER) > needs to revoke kernel usage from a whole bunch of other devices in > the same group. > > revoking kernel usage means it needs to ensure that no driver is bound > and prevent future drivers from being bound. > > iommu_setup_dma_ops() is something done once early on in boot, not at > every driver probe, so I don't see how it can help?? Note that there's some annoying inconsistency across architectures, and with the {acpi,of}_dma_configure() code in general. I guess Christoph might be thinking of the case where iommu_setup_dma_ops() *does* end up being called off the back of the bus->dma_configure() hook a few lines below the context above. iommu_setup_dma_ops() itself is indeed not the appropriate place for this (the fact that it can be called as late as driver probe is subtly broken and still on my list to fix...) but bus->dma_configure() definitely is. Only a handful of buses care about IOMMUs, and possibly even fewer of them support VFIO, so I'm in full agreement with Greg and Christoph that this absolutely warrants being scoped per-bus. I mean, we literally already have infrastructure to prevent drivers binding if the IOMMU/DMA configuration is broken or not ready yet; why would we want a totally different mechanism to prevent driver binding when the only difference is that that configuration *is* ready and working to the point that someone's already claimed it for other purposes? Robin.
On Mon, Nov 15, 2021 at 03:37:18PM +0000, Robin Murphy wrote: > IOMMUs, and possibly even fewer of them support VFIO, so I'm in full > agreement with Greg and Christoph that this absolutely warrants being scoped > per-bus. I mean, we literally already have infrastructure to prevent drivers > binding if the IOMMU/DMA configuration is broken or not ready yet; why would > we want a totally different mechanism to prevent driver binding when the > only difference is that that configuration *is* ready and working to the > point that someone's already claimed it for other purposes? I see, that does make sense I see these implementations: drivers/amba/bus.c: .dma_configure = platform_dma_configure, drivers/base/platform.c: .dma_configure = platform_dma_configure, drivers/bus/fsl-mc/fsl-mc-bus.c: .dma_configure = fsl_mc_dma_configure, drivers/pci/pci-driver.c: .dma_configure = pci_dma_configure, drivers/gpu/host1x/bus.c: .dma_configure = host1x_dma_configure, Other than host1x they all work with VFIO. Also, there is no bus->dma_unconfigure() which would be needed to restore the device as well. So, would you rather see duplicated code into the 4 drivers, and a new bus op to 'unconfigure dma' Or, a 'dev_configure_dma()' function that is roughly: if (dev->bus->dma_configure) { ret = dev->bus->dma_configure(dev); if (ret) return ret; if (!drv->suppress_auto_claim_dma_owner) { ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, NULL); if (ret) ret; } } And a pair'd undo. This is nice because we can enforce dev->bus->dma_configure when doing a user bind so everything holds together safely without relying on each bus_type to properly implement security. Jason
On Mon, Nov 15, 2021 at 11:56:13AM -0400, Jason Gunthorpe wrote: > drivers/base/platform.c: .dma_configure = platform_dma_configure, > drivers/bus/fsl-mc/fsl-mc-bus.c: .dma_configure = fsl_mc_dma_configure, > drivers/pci/pci-driver.c: .dma_configure = pci_dma_configure, > drivers/gpu/host1x/bus.c: .dma_configure = host1x_dma_configure, > > Other than host1x they all work with VFIO. > > Also, there is no bus->dma_unconfigure() which would be needed to > restore the device as well. > > So, would you rather see duplicated code into the 4 drivers, and a new > bus op to 'unconfigure dma' The tend to mostly call into common helpers eventually. > > Or, a 'dev_configure_dma()' function that is roughly: > > if (dev->bus->dma_configure) { > ret = dev->bus->dma_configure(dev); > if (ret) > return ret; > if (!drv->suppress_auto_claim_dma_owner) { > ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, > NULL); > if (ret) > ret; > } > } > > And a pair'd undo. But that seems like an even better idea to me. Even better with an early return and avoiding the pointless indentation.
On 2021-11-15 15:56, Jason Gunthorpe via iommu wrote: > On Mon, Nov 15, 2021 at 03:37:18PM +0000, Robin Murphy wrote: > >> IOMMUs, and possibly even fewer of them support VFIO, so I'm in full >> agreement with Greg and Christoph that this absolutely warrants being scoped >> per-bus. I mean, we literally already have infrastructure to prevent drivers >> binding if the IOMMU/DMA configuration is broken or not ready yet; why would >> we want a totally different mechanism to prevent driver binding when the >> only difference is that that configuration *is* ready and working to the >> point that someone's already claimed it for other purposes? > > I see, that does make sense > > I see these implementations: > > drivers/amba/bus.c: .dma_configure = platform_dma_configure, > drivers/base/platform.c: .dma_configure = platform_dma_configure, > drivers/bus/fsl-mc/fsl-mc-bus.c: .dma_configure = fsl_mc_dma_configure, > drivers/pci/pci-driver.c: .dma_configure = pci_dma_configure, > drivers/gpu/host1x/bus.c: .dma_configure = host1x_dma_configure, > > Other than host1x they all work with VFIO. > > Also, there is no bus->dma_unconfigure() which would be needed to > restore the device as well. Not if we reduce the notion of "ownership" down to "dev->iommu_group->domain != dev->iommu_group->default_domain", which I'm becoming increasingly convinced is all we actually need here. > So, would you rather see duplicated code into the 4 drivers, and a new > bus op to 'unconfigure dma' The .dma_configure flow is unavoidably a bit boilerplatey already, so personally I'd go for having the implementations call back into a common check, similarly to their current flow. That also leaves room for the bus code to further refine the outcome based on what it might know, which I can particularly imagine for cleverer buses like fsl-mc and host1x which can have lots of inside knowledge about how their devices may interact. Robin. > Or, a 'dev_configure_dma()' function that is roughly: > > if (dev->bus->dma_configure) { > ret = dev->bus->dma_configure(dev); > if (ret) > return ret; > if (!drv->suppress_auto_claim_dma_owner) { > ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, > NULL); > if (ret) > ret; > } > } > > And a pair'd undo. > > This is nice because we can enforce dev->bus->dma_configure when doing > a user bind so everything holds together safely without relying on > each bus_type to properly implement security. > > Jason > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu >
On Mon, Nov 15, 2021 at 06:35:37PM +0000, Robin Murphy wrote: > On 2021-11-15 15:56, Jason Gunthorpe via iommu wrote: > > On Mon, Nov 15, 2021 at 03:37:18PM +0000, Robin Murphy wrote: > > > > > IOMMUs, and possibly even fewer of them support VFIO, so I'm in full > > > agreement with Greg and Christoph that this absolutely warrants being scoped > > > per-bus. I mean, we literally already have infrastructure to prevent drivers > > > binding if the IOMMU/DMA configuration is broken or not ready yet; why would > > > we want a totally different mechanism to prevent driver binding when the > > > only difference is that that configuration *is* ready and working to the > > > point that someone's already claimed it for other purposes? > > > > I see, that does make sense > > > > I see these implementations: > > > > drivers/amba/bus.c: .dma_configure = platform_dma_configure, > > drivers/base/platform.c: .dma_configure = platform_dma_configure, > > drivers/bus/fsl-mc/fsl-mc-bus.c: .dma_configure = fsl_mc_dma_configure, > > drivers/pci/pci-driver.c: .dma_configure = pci_dma_configure, > > drivers/gpu/host1x/bus.c: .dma_configure = host1x_dma_configure, > > > > Other than host1x they all work with VFIO. > > > > Also, there is no bus->dma_unconfigure() which would be needed to > > restore the device as well. > > Not if we reduce the notion of "ownership" down to "dev->iommu_group->domain > != dev->iommu_group->default_domain", which I'm becoming increasingly > convinced is all we actually need here. The group will be on the default_domain regardless if a kernel driver is bound or not, so the number of bound kernel drivers still needs to be tracked and restored. > > So, would you rather see duplicated code into the 4 drivers, and a new > > bus op to 'unconfigure dma' > > The .dma_configure flow is unavoidably a bit boilerplatey already, so > personally I'd go for having the implementations call back into a common > check, similarly to their current flow. That also leaves room for the bus > code to further refine the outcome based on what it might know, which I can > particularly imagine for cleverer buses like fsl-mc and host1x which can > have lots of inside knowledge about how their devices may interact. bus specific variation does not fill me with confidence - there should not be bus specific variation on security principles, especially when the API is supporting VFIO and the like. How can we reason about that? Jason
diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h index a498ebcf4993..25d39c64c4d9 100644 --- a/include/linux/device/driver.h +++ b/include/linux/device/driver.h @@ -54,6 +54,10 @@ enum probe_type { * @owner: The module owner. * @mod_name: Used for built-in modules. * @suppress_bind_attrs: Disables bind/unbind via sysfs. + * @suppress_auto_claim_dma_owner: Disable auto claiming of kernel DMA owner. + * Drivers which don't require DMA or want to manually claim the + * owner type (e.g. userspace driver frameworks) could set this + * flag. * @probe_type: Type of the probe (synchronous or asynchronous) to use. * @of_match_table: The open firmware table. * @acpi_match_table: The ACPI match table. @@ -99,7 +103,8 @@ struct device_driver { struct module *owner; const char *mod_name; /* used for built-in modules */ - bool suppress_bind_attrs; /* disables bind/unbind via sysfs */ + bool suppress_bind_attrs:1; /* disables bind/unbind via sysfs */ + bool suppress_auto_claim_dma_owner:1; enum probe_type probe_type; const struct of_device_id *of_match_table; diff --git a/drivers/base/dd.c b/drivers/base/dd.c index 68ea1f949daa..ab3333351f19 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -28,6 +28,7 @@ #include <linux/pm_runtime.h> #include <linux/pinctrl/devinfo.h> #include <linux/slab.h> +#include <linux/iommu.h> #include "base.h" #include "power/power.h" @@ -566,6 +567,12 @@ static int really_probe(struct device *dev, struct device_driver *drv) goto done; } + if (!drv->suppress_auto_claim_dma_owner) { + ret = iommu_device_set_dma_owner(dev, DMA_OWNER_KERNEL, NULL); + if (ret) + return ret; + } + re_probe: dev->driver = drv; @@ -673,6 +680,8 @@ static int really_probe(struct device *dev, struct device_driver *drv) dev->pm_domain->dismiss(dev); pm_runtime_reinit(dev); dev_pm_set_driver_flags(dev, 0); + if (!drv->suppress_auto_claim_dma_owner) + iommu_device_release_dma_owner(dev, DMA_OWNER_KERNEL); done: return ret; } @@ -1215,6 +1224,9 @@ static void __device_release_driver(struct device *dev, struct device *parent) pm_runtime_reinit(dev); dev_pm_set_driver_flags(dev, 0); + if (!drv->suppress_auto_claim_dma_owner) + iommu_device_release_dma_owner(dev, DMA_OWNER_KERNEL); + klist_remove(&dev->p->knode_driver); device_pm_check_callbacks(dev); if (dev->bus)
This extends really_probe() to allow checking for dma ownership conflict during the driver binding process. By default, the DMA_OWNER_KERNEL is claimed for the bound driver before calling its .probe() callback. If this operation fails (e.g. the iommu group of the target device already has the DMA_OWNER_USER set), the binding process is aborted to avoid breaking the security contract for devices in the iommu group. Without this change, the vfio driver has to listen to a bus BOUND_DRIVER event and then BUG_ON() in case of dma ownership conflict. This leads to bad user experience since careless driver binding operation may crash the system if the admin overlooks the group restriction. Aside from bad design, this leads to a security problem as a root user, even with lockdown=integrity, can force the kernel to BUG. Driver may set a new flag (suppress_auto_claim_dma_owner) to disable auto claim in the binding process. Examples include kernel drivers (pci_stub, PCI bridge drivers, etc.) which don't trigger DMA at all thus can be safely exempted in DMA ownership check and userspace framework drivers (vfio/vdpa etc.) which need to manually claim DMA_OWNER_USER when assigning a device to userspace. Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Link: https://lore.kernel.org/linux-iommu/20210922123931.GI327412@nvidia.com/ Link: https://lore.kernel.org/linux-iommu/20210928115751.GK964074@nvidia.com/ Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> --- include/linux/device/driver.h | 7 ++++++- drivers/base/dd.c | 12 ++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-)