linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Joerg Roedel <joro@8bytes.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Jason Gunthorpe <jgg@nvidia.com>,
	Christoph Hellwig <hch@infradead.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Ashok Raj <ashok.raj@intel.com>, Will Deacon <will@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Dan Williams <dan.j.williams@intel.com>,
	rafael@kernel.org, Diana Craciun <diana.craciun@oss.nxp.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Eric Auger <eric.auger@redhat.com>, Liu Yi L <yi.l.liu@intel.com>,
	Jacob jun Pan <jacob.jun.pan@intel.com>,
	Chaitanya Kulkarni <kch@nvidia.com>,
	Stuart Yoder <stuyoder@gmail.com>,
	Laurentiu Tudor <laurentiu.tudor@nxp.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Li Yang <leoyang.li@nxp.com>, Dmitry Osipenko <digetx@gmail.com>,
	iommu@lists.linux-foundation.org, linux-pci@vger.kernel.org,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 02/13] driver core: Set DMA ownership during driver bind/unbind
Date: Wed, 22 Dec 2021 13:47:34 +0100	[thread overview]
Message-ID: <YcMeZlN3798noycN@kroah.com> (raw)
In-Reply-To: <20211217063708.1740334-3-baolu.lu@linux.intel.com>

On Fri, Dec 17, 2021 at 02:36:57PM +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_DMA_API 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 can force the kernel to
> BUG() even with lockdown=integrity.
> 
> 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 |  2 ++
>  drivers/base/dd.c             | 37 ++++++++++++++++++++++++++++++-----
>  2 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index a498ebcf4993..f5bf7030c416 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -54,6 +54,7 @@ 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 kernel dma auto-claim.
>   * @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.
> @@ -100,6 +101,7 @@ struct device_driver {
>  	const char		*mod_name;	/* used for built-in modules */
>  
>  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> +	bool suppress_auto_claim_dma_owner;
>  	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..b04eec5dcefa 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"
> @@ -538,6 +539,32 @@ static int call_driver_probe(struct device *dev, struct device_driver *drv)
>  	return ret;
>  }
>  
> +static int device_dma_configure(struct device *dev, struct device_driver *drv)
> +{
> +	int ret;
> +
> +	if (!dev->bus->dma_configure)
> +		return 0;
> +
> +	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_DMA_API, NULL);

Wait, the busses that wanted to configure the device, just did so in
their dma_configure callback, so why not do this type of
iommu_device_set_dma_owner() in the few busses that will want this to
happen?

Right now we only have 4 different "busses" that care about this.  Out
of the following callbacks:
	fsl_mc_dma_configure
	host1x_dma_configure
	pci_dma_configure
	platform_dma_configure

Which one will actually care about the iommu_device_set_dma_owner()
call?  All of them?  None of them?  Some of them?

Again, why can't this just happen in the (very few) bus callbacks that
care about this?  In following patches in this series, you turn off this
for the pci_dma_configure users, so what is left?  3 odd bus types that
are not used often.  How well did you test devices of those types with
this patchset?

It's fine to have "suppress" fields when they are the minority, but here
it's a _very_ tiny tiny number of actual devices in a system that will
ever get the chance to have this check happen for them and trigger,
right?

I know others told you to put this in the driver core, but I fail to see
how adding this call to the 3 busses that care about it is a lot more
work than this driver core functionality that we all will have to
maintain for forever?

> +
> +	return ret;
> +}
> +
> +static void device_dma_cleanup(struct device *dev, struct device_driver *drv)
> +{
> +	if (!dev->bus->dma_configure)
> +		return;
> +
> +	if (!drv->suppress_auto_claim_dma_owner)
> +		iommu_device_release_dma_owner(dev, DMA_OWNER_DMA_API);
> +}
> +
>  static int really_probe(struct device *dev, struct device_driver *drv)
>  {
>  	bool test_remove = IS_ENABLED(CONFIG_DEBUG_TEST_DRIVER_REMOVE) &&
> @@ -574,11 +601,8 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>  	if (ret)
>  		goto pinctrl_bind_failed;
>  
> -	if (dev->bus->dma_configure) {
> -		ret = dev->bus->dma_configure(dev);
> -		if (ret)
> -			goto probe_failed;
> -	}
> +	if (device_dma_configure(dev, drv))
> +		goto pinctrl_bind_failed;

Are you sure you are jumping to the proper error path here?  It is not
obvious why you changed this.

thanks,

greg k-h

  reply	other threads:[~2021-12-22 12:47 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-17  6:36 [PATCH v4 00/13] Fix BUG_ON in vfio_iommu_group_notifier() Lu Baolu
2021-12-17  6:36 ` [PATCH v4 01/13] iommu: Add device dma ownership set/release interfaces Lu Baolu
2021-12-17  6:36 ` [PATCH v4 02/13] driver core: Set DMA ownership during driver bind/unbind Lu Baolu
2021-12-22 12:47   ` Greg Kroah-Hartman [this message]
2021-12-22 17:52     ` Jason Gunthorpe
2021-12-23  2:08     ` Lu Baolu
2021-12-23  3:02     ` Lu Baolu
2021-12-23  7:13       ` Greg Kroah-Hartman
2021-12-23  7:23         ` Lu Baolu
2021-12-31  0:36           ` Jason Gunthorpe
2021-12-17  6:36 ` [PATCH v4 03/13] PCI: pci_stub: Suppress kernel DMA ownership auto-claiming Lu Baolu
2021-12-29 20:42   ` Bjorn Helgaas
2021-12-30  5:34     ` Lu Baolu
2021-12-30 22:24       ` Bjorn Helgaas
2021-12-31  0:40         ` Jason Gunthorpe
2021-12-31  1:10           ` Lu Baolu
2021-12-31  1:58             ` Lu Baolu
2022-01-03 19:53             ` Jason Gunthorpe
2022-01-04  1:54               ` Lu Baolu
2021-12-31  1:06         ` Lu Baolu
2021-12-17  6:36 ` [PATCH v4 04/13] PCI: portdrv: " Lu Baolu
2021-12-29 21:16   ` Bjorn Helgaas
2021-12-30  5:49     ` Lu Baolu
2021-12-17  6:37 ` [PATCH v4 05/13] iommu: Add security context management for assigned devices Lu Baolu
2021-12-17  6:37 ` [PATCH v4 06/13] iommu: Expose group variants of dma ownership interfaces Lu Baolu
2021-12-17  6:37 ` [PATCH v4 07/13] iommu: Add iommu_at[de]tach_device_shared() for multi-device groups Lu Baolu
2021-12-21 16:50   ` Robin Murphy
2021-12-21 18:46     ` Jason Gunthorpe
2021-12-22  4:22       ` Lu Baolu
2021-12-22  4:25         ` Lu Baolu
2021-12-22 20:26       ` Robin Murphy
2021-12-23  0:57         ` Jason Gunthorpe
2021-12-23  5:53           ` Lu Baolu
2021-12-23 14:03             ` Jason Gunthorpe
2021-12-24  1:30               ` Lu Baolu
2021-12-24  2:50                 ` Jason Gunthorpe
2021-12-24  6:44                   ` Lu Baolu
2022-01-04  1:53                   ` Lu Baolu
2021-12-24  3:19         ` Lu Baolu
2021-12-24 14:24           ` Jason Gunthorpe
2021-12-17  6:37 ` [PATCH v4 08/13] vfio: Set DMA USER ownership for VFIO devices Lu Baolu
2021-12-17  6:37 ` [PATCH v4 09/13] vfio: Remove use of vfio_group_viable() Lu Baolu
2021-12-17  6:37 ` [PATCH v4 10/13] vfio: Delete the unbound_list Lu Baolu
2021-12-17  6:37 ` [PATCH v4 11/13] vfio: Remove iommu group notifier Lu Baolu
2021-12-17  6:37 ` [PATCH v4 12/13] iommu: Remove iommu group changes notifier Lu Baolu
2021-12-17  6:37 ` [PATCH v4 13/13] drm/tegra: Use the iommu dma_owner mechanism Lu Baolu
2022-01-04  5:23 ` [PATCH v4 00/13] Fix BUG_ON in vfio_iommu_group_notifier() Lu Baolu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YcMeZlN3798noycN@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=airlied@linux.ie \
    --cc=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=cohuck@redhat.com \
    --cc=dan.j.williams@intel.com \
    --cc=daniel@ffwll.ch \
    --cc=diana.craciun@oss.nxp.com \
    --cc=digetx@gmail.com \
    --cc=eric.auger@redhat.com \
    --cc=hch@infradead.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jacob.jun.pan@intel.com \
    --cc=jgg@nvidia.com \
    --cc=jonathanh@nvidia.com \
    --cc=joro@8bytes.org \
    --cc=kch@nvidia.com \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=laurentiu.tudor@nxp.com \
    --cc=leoyang.li@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=stuyoder@gmail.com \
    --cc=thierry.reding@gmail.com \
    --cc=will@kernel.org \
    --cc=yi.l.liu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).