[PATCH 0/2] crypto: qat - fix config table locking

[PATCH 0/2] crypto: qat - fix config table locking

[Ahsan Atta]

The per-device configuration table is protected by cfg->lock, but not
every access to it takes that lock. adf_cfg_sec_find() walks the section
list unlocked and returns a pointer that its callers then dereference
under cfg->lock, so the section can be freed between the lookup and the
use. The dev_cfg debugfs reader walks both the section list and each
section's key-value list under a driver-global mutex that none of the
writers take, so it does not serialise against config writes, device down
or device removal, any of which can free the entry it is walking. This
series moves these accesses under the per-device cfg->lock and then drops
the redundant list walks that are no longer needed once the section is
resolved under that lock.

Note on the debugfs lock change:
The dev_cfg reader now takes a lock that lives inside the structure freed
by adf_cfg_dev_remove(). This is safe because every cleanup path calls
adf_dbgfs_exit(), which removes the dev_cfg file, before
adf_cfg_dev_remove() frees the table, and debugfs_remove() waits for
in-flight file operations to complete.

In summary:
Patch #1: Take cfg->lock around the config section lookups and switch the
dev_cfg debugfs reader to the same per-device lock. This closes the
use-after-free windows against section deletion and key updates, makes the
lookup and insert in adf_cfg_section_add() atomic so that a section can no
longer be created twice, and removes the now unused global
qat_cfg_read_lock.

Patch #2: Look up an existing key-value entry once in
adf_cfg_add_key_value_param() and act on that pointer directly. With the
section already resolved under cfg->lock, the list walks done by
adf_cfg_key_val_get() and adf_cfg_keyval_remove() are redundant, so both
those calls and the on-stack value copy are dropped. No functional change.

Ahsan Atta (2):
  crypto: qat - hold cfg->lock when accessing config sections
  crypto: qat - avoid redundant config list walks when adding a key

 drivers/crypto/intel/qat/qat_common/adf_cfg.c | 84 +++++++++----------
 1 file changed, 38 insertions(+), 46 deletions(-)


base-commit: c72ab95b5aae0f0412dc1d3284ec446ad1b315e2
-- 
2.50.1

[PATCH 1/2] crypto: qat - hold cfg->lock when accessing config sections

[Ahsan Atta]

adf_cfg_sec_find() walks the per-device section list without holding
cfg->lock, and the returned section pointer is used later under the
lock. A concurrent section deletion under cfg->lock
(adf_cfg_del_all_except() on device down, or adf_cfg_dev_remove() on
removal) can free the section first, leading to a use-after-free.

The debugfs dev_cfg reader has the same problem: qat_dev_cfg_show()
walks both the section list and each section's param_head under the
global qat_cfg_read_lock mutex, not the per-device cfg->lock used by the
add and delete paths, so the two do not serialise. Updating an existing
key frees the old key_val under cfg->lock, so even a config write
concurrent with an open dev_cfg read can free an entry mid-walk, a wider
window than section deletion alone.

Take cfg->lock around the section lookup in adf_cfg_add_key_value_param()
and around the lookup and insert in adf_cfg_section_add(), and use
down_read()/up_read(&cfg->lock) in the debugfs start/stop callbacks.
Remove the now-unused qat_cfg_read_lock.

While here, return -ENOENT rather than -EFAULT when the section is not
found; EFAULT (bad userspace address) is not meaningful for this case.

Fixes: d8cba25d2c68 ("crypto: qat - Intel(R) QAT driver framework")
Signed-off-by: Ahsan Atta <ahsan.atta@intel.com>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/intel/qat/qat_common/adf_cfg.c | 49 +++++++++++--------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/crypto/intel/qat/qat_common/adf_cfg.c b/drivers/crypto/intel/qat/qat_common/adf_cfg.c
index b88febf53a19..e13db9ede0fa 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_cfg.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_cfg.c
@@ -1,6 +1,5 @@
 // SPDX-License-Identifier: (BSD-3-Clause OR GPL-2.0-only)
 /* Copyright(c) 2014 - 2020 Intel Corporation */
-#include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/list.h>
@@ -9,13 +8,11 @@
 #include "adf_cfg.h"
 #include "adf_common_drv.h"
 
-static DEFINE_MUTEX(qat_cfg_read_lock);
-
 static void *qat_dev_cfg_start(struct seq_file *sfile, loff_t *pos)
 {
 	struct adf_cfg_device_data *dev_cfg = sfile->private;
 
-	mutex_lock(&qat_cfg_read_lock);
+	down_read(&dev_cfg->lock);
 	return seq_list_start(&dev_cfg->sec_list, *pos);
 }
 
@@ -43,7 +40,9 @@ static void *qat_dev_cfg_next(struct seq_file *sfile, void *v, loff_t *pos)
 
 static void qat_dev_cfg_stop(struct seq_file *sfile, void *v)
 {
-	mutex_unlock(&qat_cfg_read_lock);
+	struct adf_cfg_device_data *dev_cfg = sfile->private;
+
+	up_read(&dev_cfg->lock);
 }
 
 static const struct seq_operations qat_dev_cfg_sops = {
@@ -272,13 +271,10 @@ int adf_cfg_add_key_value_param(struct adf_accel_dev *accel_dev,
 				enum adf_cfg_val_type type)
 {
 	struct adf_cfg_device_data *cfg = accel_dev->cfg;
+	struct adf_cfg_section *section;
 	struct adf_cfg_key_val *key_val;
-	struct adf_cfg_section *section = adf_cfg_sec_find(accel_dev,
-							   section_name);
 	char temp_val[ADF_CFG_MAX_VAL_LEN_IN_BYTES];
-
-	if (!section)
-		return -EFAULT;
+	int ret = 0;
 
 	key_val = kzalloc_obj(*key_val);
 	if (!key_val)
@@ -308,20 +304,28 @@ int adf_cfg_add_key_value_param(struct adf_accel_dev *accel_dev,
 	 *    anything (the newly created key_val is freed).
 	 */
 	down_write(&cfg->lock);
+
+	section = adf_cfg_sec_find(accel_dev, section_name);
+	if (!section) {
+		kfree(key_val);
+		ret = -ENOENT;
+		goto unlock;
+	}
+
 	if (!adf_cfg_key_val_get(accel_dev, section_name, key, temp_val)) {
 		if (strncmp(temp_val, key_val->val, sizeof(temp_val))) {
 			adf_cfg_keyval_remove(key, section);
 		} else {
 			kfree(key_val);
-			goto out;
+			goto unlock;
 		}
 	}
 
 	adf_cfg_keyval_add(key_val, section);
 
-out:
+unlock:
 	up_write(&cfg->lock);
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(adf_cfg_add_key_value_param);
 
@@ -339,21 +343,26 @@ EXPORT_SYMBOL_GPL(adf_cfg_add_key_value_param);
 int adf_cfg_section_add(struct adf_accel_dev *accel_dev, const char *name)
 {
 	struct adf_cfg_device_data *cfg = accel_dev->cfg;
-	struct adf_cfg_section *sec = adf_cfg_sec_find(accel_dev, name);
+	struct adf_cfg_section *sec;
+	int ret = 0;
 
-	if (sec)
-		return 0;
+	down_write(&cfg->lock);
+
+	if (adf_cfg_sec_find(accel_dev, name))
+		goto unlock;
 
 	sec = kzalloc_obj(*sec);
-	if (!sec)
-		return -ENOMEM;
+	if (!sec) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
 
 	strscpy(sec->name, name);
 	INIT_LIST_HEAD(&sec->param_head);
-	down_write(&cfg->lock);
 	list_add_tail(&sec->list, &cfg->sec_list);
+unlock:
 	up_write(&cfg->lock);
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(adf_cfg_section_add);
 
-- 
2.50.1

[PATCH 2/2] crypto: qat - avoid redundant config list walks when adding a key

[Ahsan Atta]

adf_cfg_add_key_value_param() now resolves the target section under
cfg->lock, but still calls adf_cfg_key_val_get(), which re-walks the
section list to find that same section and then walks param_head to
read the current value into a temporary buffer. If the value differs it
calls adf_cfg_keyval_remove(), which walks param_head a third time to
locate the entry that was already found.

Look up the existing entry once with adf_cfg_key_value_find() and act on
that pointer directly: free the new copy if the value is unchanged, or
unlink and free the old entry before adding the new one. This drops two
redundant list walks and the on-stack temp_val copy, and removes the now
unused adf_cfg_keyval_remove() helper. No functional change.

Signed-off-by: Ahsan Atta <ahsan.atta@intel.com>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>
---
 drivers/crypto/intel/qat/qat_common/adf_cfg.c | 47 ++++++-------------
 1 file changed, 15 insertions(+), 32 deletions(-)

diff --git a/drivers/crypto/intel/qat/qat_common/adf_cfg.c b/drivers/crypto/intel/qat/qat_common/adf_cfg.c
index e13db9ede0fa..e2ef5284abef 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_cfg.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_cfg.c
@@ -145,24 +145,6 @@ static void adf_cfg_keyval_add(struct adf_cfg_key_val *new,
 	list_add_tail(&new->list, &sec->param_head);
 }
 
-static void adf_cfg_keyval_remove(const char *key, struct adf_cfg_section *sec)
-{
-	struct list_head *head = &sec->param_head;
-	struct list_head *list_ptr, *tmp;
-
-	list_for_each_prev_safe(list_ptr, tmp, head) {
-		struct adf_cfg_key_val *ptr =
-			list_entry(list_ptr, struct adf_cfg_key_val, list);
-
-		if (strncmp(ptr->key, key, sizeof(ptr->key)))
-			continue;
-
-		list_del(list_ptr);
-		kfree(ptr);
-		break;
-	}
-}
-
 static void adf_cfg_keyval_del_all(struct list_head *head)
 {
 	struct list_head *list_ptr, *tmp;
@@ -271,9 +253,8 @@ int adf_cfg_add_key_value_param(struct adf_accel_dev *accel_dev,
 				enum adf_cfg_val_type type)
 {
 	struct adf_cfg_device_data *cfg = accel_dev->cfg;
+	struct adf_cfg_key_val *key_val, *existing;
 	struct adf_cfg_section *section;
-	struct adf_cfg_key_val *key_val;
-	char temp_val[ADF_CFG_MAX_VAL_LEN_IN_BYTES];
 	int ret = 0;
 
 	key_val = kzalloc_obj(*key_val);
@@ -295,14 +276,6 @@ int adf_cfg_add_key_value_param(struct adf_accel_dev *accel_dev,
 	}
 	key_val->type = type;
 
-	/* Add the key-value pair as below policy:
-	 * 1. if the key doesn't exist, add it;
-	 * 2. if the key already exists with a different value then update it
-	 *    to the new value (the key is deleted and the newly created
-	 *    key_val containing the new value is added to the database);
-	 * 3. if the key exists with the same value, then return without doing
-	 *    anything (the newly created key_val is freed).
-	 */
 	down_write(&cfg->lock);
 
 	section = adf_cfg_sec_find(accel_dev, section_name);
@@ -312,13 +285,23 @@ int adf_cfg_add_key_value_param(struct adf_accel_dev *accel_dev,
 		goto unlock;
 	}
 
-	if (!adf_cfg_key_val_get(accel_dev, section_name, key, temp_val)) {
-		if (strncmp(temp_val, key_val->val, sizeof(temp_val))) {
-			adf_cfg_keyval_remove(key, section);
-		} else {
+	/*
+	 * Add the key-value pair as below policy:
+	 * 1. if the key doesn't exist, add it;
+	 * 2. if the key already exists with a different value then update it
+	 *    to the new value (the key is deleted and the newly created
+	 *    key_val containing the new value is added to the database);
+	 * 3. if the key exists with the same value, then return without doing
+	 *    anything (the newly created key_val is freed).
+	 */
+	existing = adf_cfg_key_value_find(section, key);
+	if (existing) {
+		if (!strncmp(existing->val, key_val->val, sizeof(existing->val))) {
 			kfree(key_val);
 			goto unlock;
 		}
+		list_del(&existing->list);
+		kfree(existing);
 	}
 
 	adf_cfg_keyval_add(key_val, section);
-- 
2.50.1

Re: [PATCH 0/2] crypto: qat - fix config table locking

[Herbert Xu]

On Fri, Sep 18, 2026 at 01:41:10PM +0100, Ahsan Atta wrote:
> The per-device configuration table is protected by cfg->lock, but not
> every access to it takes that lock. adf_cfg_sec_find() walks the section
> list unlocked and returns a pointer that its callers then dereference
> under cfg->lock, so the section can be freed between the lookup and the
> use. The dev_cfg debugfs reader walks both the section list and each
> section's key-value list under a driver-global mutex that none of the
> writers take, so it does not serialise against config writes, device down
> or device removal, any of which can free the entry it is walking. This
> series moves these accesses under the per-device cfg->lock and then drops
> the redundant list walks that are no longer needed once the section is
> resolved under that lock.
> 
> Note on the debugfs lock change:
> The dev_cfg reader now takes a lock that lives inside the structure freed
> by adf_cfg_dev_remove(). This is safe because every cleanup path calls
> adf_dbgfs_exit(), which removes the dev_cfg file, before
> adf_cfg_dev_remove() frees the table, and debugfs_remove() waits for
> in-flight file operations to complete.
> 
> In summary:
> Patch #1: Take cfg->lock around the config section lookups and switch the
> dev_cfg debugfs reader to the same per-device lock. This closes the
> use-after-free windows against section deletion and key updates, makes the
> lookup and insert in adf_cfg_section_add() atomic so that a section can no
> longer be created twice, and removes the now unused global
> qat_cfg_read_lock.
> 
> Patch #2: Look up an existing key-value entry once in
> adf_cfg_add_key_value_param() and act on that pointer directly. With the
> section already resolved under cfg->lock, the list walks done by
> adf_cfg_key_val_get() and adf_cfg_keyval_remove() are redundant, so both
> those calls and the on-stack value copy are dropped. No functional change.
> 
> Ahsan Atta (2):
>   crypto: qat - hold cfg->lock when accessing config sections
>   crypto: qat - avoid redundant config list walks when adding a key
> 
>  drivers/crypto/intel/qat/qat_common/adf_cfg.c | 84 +++++++++----------
>  1 file changed, 38 insertions(+), 46 deletions(-)
> 
> 
> base-commit: c72ab95b5aae0f0412dc1d3284ec446ad1b315e2
> -- 
> 2.50.1

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt