Re: [PATCH] dmaengine: idxd: Ensure safe user copy of completion record

From: Fenghua Yu
Date: Fri Feb 09 2024 - 19:21:05 EST


Hi, Lijun,

On 2/9/24 13:53, Lijun Pan wrote:


On 2/10/2024 3:14 AM, Fenghua Yu wrote:
If CONFIG_HARDENED_USERCOPY is enabled, copying completion record from
event log cache to user triggers a kernel bug.

..

Fixes: c2f156bf168f ("dmaengine: idxd: create kmem cache for event log fault items")
Reported-by: Tony Zhu <tony.zhu@xxxxxxxxx>
Tested-by: Tony Zhu <tony.zhu@xxxxxxxxx>
Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>

Reviewed-by: Lijun Pan <lijun.pan@xxxxxxxxx>

---
  drivers/dma/idxd/init.c | 15 ++++++++++++---
  1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 14df1f1347a8..4954adc6bb60 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -343,7 +343,9 @@ static void idxd_cleanup_internals(struct idxd_device *idxd)
  static int idxd_init_evl(struct idxd_device *idxd)
  {
      struct device *dev = &idxd->pdev->dev;
+    unsigned int evl_cache_size;
      struct idxd_evl *evl;
+    const char *idxd_name;
      if (idxd->hw.gen_cap.evl_support == 0)
          return 0;
@@ -355,9 +357,16 @@ static int idxd_init_evl(struct idxd_device *idxd)
      spin_lock_init(&evl->lock);
      evl->size = IDXD_EVL_SIZE_MIN;
-    idxd->evl_cache = kmem_cache_create(dev_name(idxd_confdev(idxd)),
-                        sizeof(struct idxd_evl_fault) + evl_ent_size(idxd),
-                        0, 0, NULL);
+    idxd_name = dev_name(idxd_confdev(idxd));
+    evl_cache_size = sizeof(struct idxd_evl_fault) + evl_ent_size(idxd);
+    /*
+     * Since completion record in evl_cache will be copied to user
+     * when handling completion record page fault, need to create
+     * the cache suitable for user copy.
+     */

Maybe briefly compare kmem_cache_create() with kmem_cache_create_usercopy() and add up to the above comments. If you think it too verbose, then forget about it.

It's improper to add comment to compare the two functions here because:
1. When people look into this code in init.c, they have no idea why compare the functions here when only kmem_cache_create_usercopy() is used. The comparison is only meaningful in this patch's context and has been explained in the patch commit message.

2. Comparison or any details of the function can be found easily in the function implementation. No need to add more details on top of the current comment which covers enough info (i.e. why call this function) already.

Given the above reasons, I will keep the current comment and patch without change.

Thanks.

-Fenghua