[PATCH] dmaengine: idxd: Do not enable user type Work Queue without Shared Virtual Addressing

[PATCH] dmaengine: idxd: Do not enable user type Work Queue without Shared Virtual Addressing

[Fenghua Yu]

Userspace can directly access physical address through user type
Work Queue (WQ) in two scenarios: no IOMMU or IOMMU Passthrough
without Shared Virtual Addressing (SVA). In these two cases, user type WQ
allows userspace to issue DMA physical address access without virtual
to physical translation.

This is inconsistent with the security goals of a good kernel API.

Plus there is no usage for user type WQ without SVA.

So enable user type WQ only when SVA is enabled (i.e. user PASID is
enabled).

Fixes: 42d279f9137a ("dmaengine: idxd: add char driver to expose submission portal to userland")

Suggested-by: Arjan Van De Ven <arjan.van.de.ven@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/idxd/cdev.c   | 14 ++++++++++++++
 include/uapi/linux/idxd.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
index c2808fd081d6..4cd3400c5a48 100644
--- a/drivers/dma/idxd/cdev.c
+++ b/drivers/dma/idxd/cdev.c
@@ -312,6 +312,20 @@ static int idxd_user_drv_probe(struct idxd_dev *idxd_dev)
 	if (idxd->state != IDXD_DEV_ENABLED)
 		return -ENXIO;
 
+	/*
+	 * User type WQ is enabled only when SVA is enabled for two reasons:
+	 *   - If no IOMMU or IOMMU Passthrough without SVA, userspace
+	 *     can directly access physical address through the WQ.
+	 *   - There is no usage case for the WQ without SVA.
+	 */
+	if (!device_user_pasid_enabled(idxd)) {
+		idxd->cmd_status = IDXD_SCMD_WQ_USER_NO_IOMMU;
+		dev_dbg(&idxd->pdev->dev,
+			"User type WQ cannot be enabled without SVA.\n");
+
+		return -EOPNOTSUPP;
+	}
+
 	mutex_lock(&wq->wq_lock);
 	wq->type = IDXD_WQT_USER;
 	rc = drv_enable_wq(wq);
diff --git a/include/uapi/linux/idxd.h b/include/uapi/linux/idxd.h
index 095299c75828..2b9e7feba3f3 100644
--- a/include/uapi/linux/idxd.h
+++ b/include/uapi/linux/idxd.h
@@ -29,6 +29,7 @@ enum idxd_scmd_stat {
 	IDXD_SCMD_WQ_NO_SIZE = 0x800e0000,
 	IDXD_SCMD_WQ_NO_PRIV = 0x800f0000,
 	IDXD_SCMD_WQ_IRQ_ERR = 0x80100000,
+	IDXD_SCMD_WQ_USER_NO_IOMMU = 0x80110000,
 };
 
 #define IDXD_SCMD_SOFTERR_MASK	0x80000000
-- 
2.32.0

Re: [PATCH] dmaengine: idxd: Do not enable user type Work Queue without Shared Virtual Addressing

[Dave Hansen]

On 10/12/22 13:14, Fenghua Yu wrote:
> Userspace can directly access physical address through user type
> Work Queue (WQ) in two scenarios: no IOMMU or IOMMU Passthrough
> without Shared Virtual Addressing (SVA). In these two cases, user type WQ
> allows userspace to issue DMA physical address access without virtual
> to physical translation.
> 
> This is inconsistent with the security goals of a good kernel API.
> 
> Plus there is no usage for user type WQ without SVA.
> 
> So enable user type WQ only when SVA is enabled (i.e. user PASID is
> enabled).

I'm not sure the changelog here is great.

The whole "user Work Queue" thing is an entire *DRIVER*.  So, this
really has zero to do with the type of workqueue and everything to do
with the kind of drivers we allow to be loaded and drive the hardware.

Basically, the *hardware* allows pretty arbitrary direct access to
physical memory.  The 'idxd_user_drv' driver code (including
idxd_user_drv_probe()) gives low-level, direct access to the hardware,
which is bad news.

Plus, even if userspace got access to the device via this driver, they
have to feel physical addresses to it, which is generally not easy from
userspace.

That's as close as I can get to rephrasing the above TLA soup in plain
old English.

I also detest the "There is no usage case for the WQ without SVA."
language.  Those words lack meaning.  There has to be a *REASON* there
is no use case.  Please think about what those words *mean*, then delete
them and write what they mean.

RE: [PATCH] dmaengine: idxd: Do not enable user type Work Queue without Shared Virtual Addressing

[Yu, Fenghua]

Hi, Dave,

On 10/12/22 14:14, Dave Hansen wrote:
> On 10/12/22 13:14, Fenghua Yu wrote:
> > Userspace can directly access physical address through user type Work
> > Queue (WQ) in two scenarios: no IOMMU or IOMMU Passthrough without
> > Shared Virtual Addressing (SVA). In these two cases, user type WQ
> > allows userspace to issue DMA physical address access without virtual
> > to physical translation.
> >
> > This is inconsistent with the security goals of a good kernel API.
> >
> > Plus there is no usage for user type WQ without SVA.
> >
> > So enable user type WQ only when SVA is enabled (i.e. user PASID is
> > enabled).
> 
> I'm not sure the changelog here is great.
> 
> The whole "user Work Queue" thing is an entire *DRIVER*.  So, this really has
> zero to do with the type of workqueue and everything to do with the kind of
> drivers we allow to be loaded and drive the hardware.
> 
> Basically, the *hardware* allows pretty arbitrary direct access to physical
> memory.  The 'idxd_user_drv' driver code (including
> idxd_user_drv_probe()) gives low-level, direct access to the hardware, which is
> bad news.
> 
> Plus, even if userspace got access to the device via this driver, they have to feel
> physical addresses to it, which is generally not easy from userspace.
> 
> That's as close as I can get to rephrasing the above TLA soup in plain old English.
> 
> I also detest the "There is no usage case for the WQ without SVA."
> language.  Those words lack meaning.  There has to be a *REASON* there is no
> use case.  Please think about what those words *mean*, then delete them and
> write what they mean.

I'm working on changing the changelog and will send v2 with the changes.

Thanks.

-Fenghua