Date:   Mon, 15 Nov 2021 17:19:52 -0400
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Robin Murphy <robin.murphy@arm.com>
Cc:     Christoph Hellwig <hch@infradead.org>,
        Kevin Tian <kevin.tian@intel.com>,
        Chaitanya Kulkarni <kch@nvidia.com>,
        Ashok Raj <ashok.raj@intel.com>, kvm@vger.kernel.org,
        rafael@kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Cornelia Huck <cohuck@redhat.com>,
        Will Deacon <will@kernel.org>, linux-kernel@vger.kernel.org,
        iommu@lists.linux-foundation.org,
        Alex Williamson <alex.williamson@redhat.com>,
        Jacob jun Pan <jacob.jun.pan@intel.com>,
        linux-pci@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
        Diana Craciun <diana.craciun@oss.nxp.com>
Subject: Re: [PATCH 03/11] PCI: pci_stub: Suppress kernel DMA ownership
 auto-claiming
Message-ID: <20211115211952.GA2534655@nvidia.com>
References: <20211115020552.2378167-1-baolu.lu@linux.intel.com>
 <20211115020552.2378167-4-baolu.lu@linux.intel.com>
 <YZJe1jquP+osF+Wn@infradead.org>
 <20211115133107.GB2379906@nvidia.com>
 <495c65e4-bd97-5f29-d39b-43671acfec78@arm.com>
 <20211115161756.GP2105516@nvidia.com>
 <e9db18d3-dea3-187a-d58a-31a913d95211@arm.com>
 <20211115192212.GQ2105516@nvidia.com>
 <ab3ae3d1-c4d7-251a-fecc-d21f6b9d87a5@arm.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ab3ae3d1-c4d7-251a-fecc-d21f6b9d87a5@arm.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?K3htXgdc7AiBe73aalH1rN9LO5gickbyqk5hkk1UM7O+b4JVSo3B6w7FDOyh?=
 =?us-ascii?Q?8sD2DWj80nvLsgDrupYVWY6HwauEiPWzJdVl4sWW1oTJ1L8bH+rDA3WZ5YKw?=
 =?us-ascii?Q?rPI7GRwBIiuZ7Wl2HMdNP0zyy79yDXO1cjjd1Ie0qkZJcqxR/jM4UtXUEiFq?=
 =?us-ascii?Q?FfTfPPDEcJuvbKIsKtnM7D3fQ7PSd4CXUZPepLbBa1IwNdW+HvW32/MzLntu?=
 =?us-ascii?Q?puW2mJG4sCFHh8m9rzPC8K+pYobK28JdcnqunIBibw5O423HnGnz2HCHTh/C?=
 =?us-ascii?Q?EF9qAMWoIMUqU9wXxTay3LLKG469BXCQaE0xWTfMBcY5tOU3D96bm16s0csm?=
 =?us-ascii?Q?Gyj0qs/NuexEyHWcOqraasjvrnOGH0hgfVhMMjZQn4zzNmJWnAY9aHjlifjg?=
 =?us-ascii?Q?muLHZMWAX8tEo/yGzhHQ1HP3F6DsSGmCSUAwxsEQpPTK06m5ugwEZ9AFKjX4?=
 =?us-ascii?Q?QN5p51TQt6JTOEFR+qcx4wiuK6I+eIKqlRdGNnpcP//y/07JSswiedttGpT8?=
 =?us-ascii?Q?D8NFHOKDcwPee+bm5Ue6phPbog7/nHiQUq0PXFInE2/M4j4rzvKN4BLTYN/i?=
 =?us-ascii?Q?1df+swRDJsQMu/kzv9MhtIktJ/yBnUdmQeYz0rilETgPXpS0VcZ2J8oW7Kpy?=
 =?us-ascii?Q?zIg1tiOZaRsKImkf5fIdhuG2295K3K4OVXuZKMLxt6e+qUmK8jTK5D8u7I4v?=
 =?us-ascii?Q?pGt5fPpsRVQWIqP6HTop76HGvIr0RAkB9CWDsiPGDv+7m1s8tT7dS5/w4m1y?=
 =?us-ascii?Q?QyuxweY7EAX8hZ4NrYPZTmFBPtlD9A7bR08/M5mnBrJIiVxG1KTyG1Sh3PsG?=
 =?us-ascii?Q?0RmrtGoCIs9P0CMr/pZUGbtATcOl9DhV2UzVLNfatDHS2E1GdqBnrFRmtpT7?=
 =?us-ascii?Q?k/g3H59O5laC5/sQ8JgSKdYnfESaAnVJK0brmP1n2m9gUeRmrwNJxT5UJ5bO?=
 =?us-ascii?Q?mSgCvV46hh9nMXTBZPwjuLXB/xNsheqDIEDhTAP2IKtw/X0Wxyff0Z7g+Gyr?=
 =?us-ascii?Q?Aq+0YLem3RWYKv60zaYGM2oeMX/5AnNsd/ZRp11c9Hw/JN9Uv0c83H1G80FV?=
 =?us-ascii?Q?MtCndDUOe62mNjnuq+aWKlzOLOI2FFXnz1ERWsadYkLJsb0NGh77D15NFEsv?=
 =?us-ascii?Q?MVyndWBqt4nkM5/V1+GGG21lVqcx93Vjj6ZF2uGm3oLB04fU9+PV5zdyGa4Z?=
 =?us-ascii?Q?mkDkl1YuxqZJSM9Pu0gvat1BNmr1Or4yqTUad99iVfHzAehCpJIkiIvOy5Jl?=
 =?us-ascii?Q?oAoLfLGeg6hjz2elPuv5Z+Bvk0apd+D2zSDa6OVZuHng0G7SzTLiVPWn14ps?=
 =?us-ascii?Q?cciOhW5Jxcqq6C7rI0G5S1PQYAFsXq+IEiOEK442i8aHfZB5RI6vRdMS0lsl?=
 =?us-ascii?Q?KbeM4cdnuF3XN7RAePeOFRdDGgpH4m+6vOHv3pKOZDf6dGaa97UCmT+Yi6RM?=
 =?us-ascii?Q?SEGuPWHCz8J6yZ+05xonmt1zxJ4EzLiFmm1dR4L0E7FaZFe7PJ5ElgpQeQE+?=
 =?us-ascii?Q?b0bGY/LxrBSLjSiJY8Sq2CdZ2cAWDQZzfbSRspBQe9NxvkZgeG/7eUqNmEGq?=
 =?us-ascii?Q?RNo/MIocKEXte3gCMTk=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ad8d6868-7118-4a01-e1a7-08d9a87daabb
X-MS-Exchange-CrossTenant-AuthSource: BL0PR12MB5506.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Nov 2021 21:19:54.0266
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: BFn5XgPdAT6K0PcFiARHz+LdZgOKUNJE5eBkF9ZyeJqzNUTwgzSXXlzRb1Vcr1zK
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5208
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, Nov 15, 2021 at 08:58:19PM +0000, Robin Murphy wrote:
> > The above scenarios are already blocked by the kernel with
> > LOCKDOWN_DEV_MEM - yes there are historical ways to violate kernel
> > integrity, and these days they almost all have mitigation. I would
> > consider any kernel integrity violation to be a bug today if
> > LOCKDOWN_INTEGRITY_MAX is enabled.
> > 
> > I don't know why you bring this up?
> 
> Because as soon as anyone brings up "security" I'm not going to blindly
> accept the implicit assumption that VFIO is the only possible way to get one
> device to mess with another. That was just a silly example in the most basic
> terms, and obviously I don't expect well-worn generic sysfs interfaces to be
> a genuine threat, but how confident are you that no other subsystem- or
> driver-level interfaces past present and future can ever be tricked into p2p
> DMA?

Given the definition of LOCKDOWN_INTEGRITY_MAX I will consider any
past/present/future p2p attacks as definitive kernel bugs.

Generally, allowing a device to do arbitary DMA to a userspace
controlled address is a pretty serious bug, and directly attacking the
kernel memory is a much more interesting and serious threat vector.

> > What is the preference then? This is the only working API today,
> > right?
> 
> I believe the intent was that everyone should move to
> iommu_group_get()/iommu_attach_group() - precisely *because*
> iommu_attach_device() can't work sensibly for multi-device groups.

And iommu_attach_group() can't work sensibly for anything except VFIO
today, so hum :)

> > > Indeed I wasn't imagining it changing any ownership, just preventing a group
> > > from being attached to a non-default domain if it contains devices bound to
> > > different incompatible drivers.
> > 
> > So this could solve just the domain/DMA API problem, but it leaves the
> > MMIO peer-to-peer issue unsolved, and it gives no tools to solve it in
> > a layered way.
> > 
> > This seems like half an idea, do you have a solution for the rest?
> 
> Tell me how the p2p DMA issue can manifest if device A is prohibited from
> attaching to VFIO's unmanaged domain while device B still has a driver
> bound, and thus would fail to be assigned to userspace in the first place.
> And conversely if non-VFIO drivers are still prevented from binding to
> device B while device A remains attached to the VFIO domain.

You've assumed that a group is continuously attached to the same
domain during the entire period that userspace has MMIO.

Any domain detatch creates a race where a kernel driver can jump in
and bind, while user space continues to have MMIO control over a
device. That violates the security invariant.

Many new flows, like PASID support, are requiring dynamically changing
the domain bound to a group.

If you want to go in this direction then we also need to have some
kind of compare and swap operation for the domain currently bound to a
group.

From a security perspective I disliek this idea a lot. Instead of
having nice clear barriers indicating the security domain we have a
very subtle 'so long as a domain is attached' idea, which is going to
get broken.

> > The concept of DMA USER is important here, and it is more than just
> > which domain is attached.
> 
> Tell me how a device would be assigned to userspace while its group is still
> attached to a kernel-managed default domain.
> 
> As soon as anyone calls iommu_attach_group() - or indeed
> iommu_attach_device() if more devices may end up hotplugged into the same
> group later - *that's* when the door opens for potential subversion of any
> kind, without ever having to leave kernel space.

The approach in this series can solve both, attach_device could switch
the device to user mode and it will block future hot plugged kernel
drivers.

> > VFIO also has logic related to the file
> 
> Yes, because unsurprisingly VFIO code is tailored for the specific case of
> VFIO usage rather than anything more general.

VFIO represents this class of users exposing the IOMMU to userspace,
I say it is general of that use class.

> > It isn't muddying the water, it is providing common security code that
> > is easy to undertand.
> > 
> > *Which* userspace FD/process owns the iommu_group is important
> > security information because we can't have process A do DMA attacks on
> > some other process B.
> 
> Tell me how a single group could be attached to two domains representing two
> different process address spaces at once.

Again you are focused on domains and ignoring MMIO.

Requiring the users of the API to continuously assert a non-default
domain is a non-trivial ask.

> In case this concept wasn't as clear as I thought, which seems to be so:
>
>                  | dev->iommu_group->domain | dev->driver
> DMA_OWNER_NONE   |          default         |   unbound
> DMA_OWNER_KERNEL |          default         |    bound
> DMA_OWNER_USER   |        non-default       |    bound

Unfortunately this can't use dev->driver. Reading dev->driver of every
device in the group requires holding the device_lock. really_probe()
already holds the device lock so this becomes a user triggerable ABBA
deadlock when scaled up to N devices.

This is why this series uses only the group mutex and tracks if
drivers are bound inside the group. I view that as unavoidable.

Jason