* Re: [PATCH] dm: introduce spinlocks to prevent concurrent usage and release of device_metadata and hash
2025-07-17 14:07 [PATCH] dm: introduce spinlocks to prevent concurrent usage and release of device_metadata and hash Li Lingfeng
@ 2025-07-17 9:43 ` Hou Tao
2025-07-17 18:45 ` Mikulas Patocka
0 siblings, 1 reply; 4+ messages in thread
From: Hou Tao @ 2025-07-17 9:43 UTC (permalink / raw)
To: Li Lingfeng, dm-devel
Cc: agk, snitzer, mpatocka, tusharsu, colin.i.king, linux-kernel,
yi.zhang, yangerkun, yukuai3, chengzhihao1, lilingfeng
Hi,
On 7/17/2025 10:07 PM, Li Lingfeng wrote:
> There is a window between freeing md->ima.active_table.hash and setting
> md->ima.active_table.hash to NULL in dm_ima_measure_on_device_resume().
> If dm_ima_measure_on_device_remove() accesses md->ima.active_table.hash
> concurrently during this window, it could lead to a double free or UAF,
> as shown below:
The UAF is a problem. However, I think the order of
dm_ima_measure_on_device_remove() and dm_ima_measure_on_device_resume is
bigger problem. Is it possible to order these two invocations properly
by using suspend_lock ?
>
> BUG: KASAN: slab-use-after-free in dm_ima_measure_on_device_remove+0x3fc/0x6c0
> Read of size 71 at addr ffff88817bb9e220 by task dmsetup/2303
>
> CPU: 2 UID: 0 PID: 2303 Comm: dmsetup Not tainted 6.16.0-rc6-dirty #7 PREEMPT(none)
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> Call Trace:
> <TASK>
> dump_stack_lvl+0x5b/0x80
> print_address_description.constprop.0+0x88/0x310
> print_report+0x12f/0x21d
> kasan_report+0xcc/0x190
> kasan_check_range+0x104/0x1b0
> __asan_memcpy+0x23/0x60
> dm_ima_measure_on_device_remove+0x3fc/0x6c0
> dev_remove+0x123/0x1e0
> ctl_ioctl+0x2a2/0x480
> dm_ctl_ioctl+0xe/0x20
> __x64_sys_ioctl+0xc7/0x110
> do_syscall_64+0x72/0x390
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> To reproduce this issue, add a delay between freeing
> md->ima.active_table.hash and setting it to NULL, using the following
> steps:
> dmsetup create mydevice --table "0 2097152 linear /dev/sda 0"
> dmsetup suspend mydevice
> dmsetup reload mydevice --table "0 2097152 linear /dev/sdb 0"
> dmsetup resume mydevice &
> dmsetup remove mydevice
>
> The device_metadata also appears to have a similar issue.
> Add spinlocks for hash and device_metadata to prevent concurrent
> usage and release.
Instead of adding four spin-locks, how about only using one ?
>
> Fixes: 91ccbbac1747 ("dm ima: measure data on table load")
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
> drivers/md/dm-ima.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> drivers/md/dm-ima.h | 2 ++
> 2 files changed, 47 insertions(+)
>
> diff --git a/drivers/md/dm-ima.c b/drivers/md/dm-ima.c
> index b90f34259fbb..ff2ac54addb8 100644
> --- a/drivers/md/dm-ima.c
> +++ b/drivers/md/dm-ima.c
> @@ -170,6 +170,10 @@ void dm_ima_reset_data(struct mapped_device *md)
> {
> memset(&(md->ima), 0, sizeof(md->ima));
> md->ima.dm_version_str_len = strlen(DM_IMA_VERSION_STR);
> + spin_lock_init(&md->ima.active_table.device_metadata_lock);
> + spin_lock_init(&md->ima.inactive_table.device_metadata_lock);
> + spin_lock_init(&md->ima.active_table.hash_lock);
> + spin_lock_init(&md->ima.inactive_table.hash_lock);
> }
>
> /*
> @@ -336,19 +340,24 @@ void dm_ima_measure_on_table_load(struct dm_table *table, unsigned int status_fl
> for (i = 0; i < digest_size; i++)
> snprintf((digest_buf + hash_alg_prefix_len + (i*2)), 3, "%02x", digest[i]);
>
> + spin_lock(&table->md->ima.inactive_table.hash_lock);
> if (table->md->ima.active_table.hash != table->md->ima.inactive_table.hash)
> kfree(table->md->ima.inactive_table.hash);
>
> table->md->ima.inactive_table.hash = digest_buf;
> table->md->ima.inactive_table.hash_len = strlen(digest_buf);
> + spin_unlock(&table->md->ima.inactive_table.hash_lock);
> +
> table->md->ima.inactive_table.num_targets = num_targets;
>
> + spin_lock(&table->md->ima.inactive_table.device_metadata_lock);
> if (table->md->ima.active_table.device_metadata !=
> table->md->ima.inactive_table.device_metadata)
> kfree(table->md->ima.inactive_table.device_metadata);
>
> table->md->ima.inactive_table.device_metadata = device_data_buf;
> table->md->ima.inactive_table.device_metadata_len = device_data_buf_len;
> + spin_unlock(&table->md->ima.inactive_table.device_metadata_lock);
>
> goto exit;
> error:
> @@ -388,18 +397,22 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap)
> l += md->ima.dm_version_str_len;
>
> if (swap) {
> + spin_lock(&md->ima.active_table.hash_lock);
> if (md->ima.active_table.hash != md->ima.inactive_table.hash)
> kfree(md->ima.active_table.hash);
>
> md->ima.active_table.hash = NULL;
> md->ima.active_table.hash_len = 0;
> + spin_unlock(&md->ima.active_table.hash_lock);
>
> + spin_lock(&md->ima.active_table.device_metadata_lock);
> if (md->ima.active_table.device_metadata !=
> md->ima.inactive_table.device_metadata)
> kfree(md->ima.active_table.device_metadata);
>
> md->ima.active_table.device_metadata = NULL;
> md->ima.active_table.device_metadata_len = 0;
> + spin_unlock(&md->ima.active_table.device_metadata_lock);
> md->ima.active_table.num_targets = 0;
>
> if (md->ima.inactive_table.hash) {
> @@ -429,6 +442,7 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap)
> nodata = false;
> }
>
> + spin_lock(&md->ima.active_table.hash_lock);
> if (md->ima.active_table.hash) {
> memcpy(device_table_data + l, active, active_len);
> l += active_len;
> @@ -442,6 +456,7 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap)
>
> nodata = false;
> }
> + spin_unlock(&md->ima.active_table.hash_lock);
>
> if (nodata) {
> r = dm_ima_alloc_and_copy_name_uuid(md, &dev_name, &dev_uuid, noio);
> @@ -503,6 +518,7 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
> memcpy(device_table_data + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
> l += md->ima.dm_version_str_len;
>
> + spin_lock(&md->ima.active_table.device_metadata_lock);
> if (md->ima.active_table.device_metadata) {
> memcpy(device_table_data + l, device_active_str, device_active_len);
> l += device_active_len;
> @@ -513,7 +529,9 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
>
> nodata = false;
> }
> + spin_unlock(&md->ima.active_table.device_metadata_lock);
>
> + spin_lock(&md->ima.inactive_table.device_metadata_lock);
> if (md->ima.inactive_table.device_metadata) {
> memcpy(device_table_data + l, device_inactive_str, device_inactive_len);
> l += device_inactive_len;
> @@ -524,7 +542,9 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
>
> nodata = false;
> }
> + spin_unlock(&md->ima.inactive_table.device_metadata_lock);
>
> + spin_lock(&md->ima.active_table.hash_lock);
> if (md->ima.active_table.hash) {
> memcpy(device_table_data + l, active_table_str, active_table_len);
> l += active_table_len;
> @@ -538,7 +558,9 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
>
> nodata = false;
> }
> + spin_unlock(&md->ima.active_table.hash_lock);
>
> + spin_lock(&md->ima.inactive_table.hash_lock);
> if (md->ima.inactive_table.hash) {
> memcpy(device_table_data + l, inactive_table_str, inactive_table_len);
> l += inactive_table_len;
> @@ -552,6 +574,7 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
>
> nodata = false;
> }
> + spin_unlock(&md->ima.inactive_table.hash_lock);
> /*
> * In case both active and inactive tables, and corresponding
> * device metadata is cleared/missing - record the name and uuid
> @@ -582,17 +605,31 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
> kfree(device_table_data);
> kfree(capacity_str);
> exit:
> + spin_lock(&md->ima.active_table.device_metadata_lock);
> + spin_lock(&md->ima.inactive_table.device_metadata_lock);
> kfree(md->ima.active_table.device_metadata);
>
> if (md->ima.active_table.device_metadata !=
> md->ima.inactive_table.device_metadata)
> kfree(md->ima.inactive_table.device_metadata);
>
> + md->ima.active_table.device_metadata = NULL;
> + md->ima.inactive_table.device_metadata = NULL;
> + spin_unlock(&md->ima.inactive_table.device_metadata_lock);
> + spin_unlock(&md->ima.active_table.device_metadata_lock);
> +
> + spin_lock(&md->ima.active_table.hash_lock);
> + spin_lock(&md->ima.inactive_table.hash_lock);
> kfree(md->ima.active_table.hash);
>
> if (md->ima.active_table.hash != md->ima.inactive_table.hash)
> kfree(md->ima.inactive_table.hash);
>
> + md->ima.active_table.hash = NULL;
> + md->ima.inactive_table.hash = NULL;
> + spin_unlock(&md->ima.inactive_table.hash_lock);
> + spin_unlock(&md->ima.active_table.hash_lock);
> +
> dm_ima_reset_data(md);
>
> kfree(dev_name);
> @@ -623,6 +660,8 @@ void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map)
> memcpy(device_table_data + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
> l += md->ima.dm_version_str_len;
>
> + spin_lock(&md->ima.inactive_table.hash_lock);
> + spin_lock(&md->ima.inactive_table.device_metadata_lock);
> if (md->ima.inactive_table.device_metadata_len &&
> md->ima.inactive_table.hash_len) {
> memcpy(device_table_data + l, md->ima.inactive_table.device_metadata,
> @@ -642,6 +681,8 @@ void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map)
>
> nodata = false;
> }
> + spin_unlock(&md->ima.inactive_table.device_metadata_lock);
> + spin_unlock(&md->ima.inactive_table.hash_lock);
>
> if (nodata) {
> if (dm_ima_alloc_and_copy_name_uuid(md, &dev_name, &dev_uuid, noio))
> @@ -660,19 +701,23 @@ void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map)
> dm_ima_measure_data("dm_table_clear", device_table_data, l, noio);
>
> if (new_map) {
> + spin_lock(&md->ima.inactive_table.hash_lock);
> if (md->ima.inactive_table.hash &&
> md->ima.inactive_table.hash != md->ima.active_table.hash)
> kfree(md->ima.inactive_table.hash);
>
> md->ima.inactive_table.hash = NULL;
> md->ima.inactive_table.hash_len = 0;
> + spin_unlock(&md->ima.inactive_table.hash_lock);
>
> + spin_lock(&md->ima.inactive_table.device_metadata_lock);
> if (md->ima.inactive_table.device_metadata &&
> md->ima.inactive_table.device_metadata != md->ima.active_table.device_metadata)
> kfree(md->ima.inactive_table.device_metadata);
>
> md->ima.inactive_table.device_metadata = NULL;
> md->ima.inactive_table.device_metadata_len = 0;
> + spin_unlock(&md->ima.inactive_table.device_metadata_lock);
> md->ima.inactive_table.num_targets = 0;
>
> if (md->ima.active_table.hash) {
> diff --git a/drivers/md/dm-ima.h b/drivers/md/dm-ima.h
> index 568870a1a145..a021056f4ab5 100644
> --- a/drivers/md/dm-ima.h
> +++ b/drivers/md/dm-ima.h
> @@ -35,6 +35,7 @@ struct dm_ima_device_table_metadata {
> * pairs delimited by a semicolon at the end of the list.
> */
> char *device_metadata;
> + spinlock_t device_metadata_lock;
> unsigned int device_metadata_len;
> unsigned int num_targets;
>
> @@ -43,6 +44,7 @@ struct dm_ima_device_table_metadata {
> * attributes' key-value pairs from the active/inactive tables.
> */
> char *hash;
> + spinlock_t hash_lock;
> unsigned int hash_len;
> };
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] dm: introduce spinlocks to prevent concurrent usage and release of device_metadata and hash
@ 2025-07-17 14:07 Li Lingfeng
2025-07-17 9:43 ` Hou Tao
0 siblings, 1 reply; 4+ messages in thread
From: Li Lingfeng @ 2025-07-17 14:07 UTC (permalink / raw)
To: dm-devel
Cc: agk, snitzer, mpatocka, tusharsu, colin.i.king, linux-kernel,
houtao1, yi.zhang, yangerkun, yukuai3, chengzhihao1, lilingfeng3,
lilingfeng
There is a window between freeing md->ima.active_table.hash and setting
md->ima.active_table.hash to NULL in dm_ima_measure_on_device_resume().
If dm_ima_measure_on_device_remove() accesses md->ima.active_table.hash
concurrently during this window, it could lead to a double free or UAF,
as shown below:
BUG: KASAN: slab-use-after-free in dm_ima_measure_on_device_remove+0x3fc/0x6c0
Read of size 71 at addr ffff88817bb9e220 by task dmsetup/2303
CPU: 2 UID: 0 PID: 2303 Comm: dmsetup Not tainted 6.16.0-rc6-dirty #7 PREEMPT(none)
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x5b/0x80
print_address_description.constprop.0+0x88/0x310
print_report+0x12f/0x21d
kasan_report+0xcc/0x190
kasan_check_range+0x104/0x1b0
__asan_memcpy+0x23/0x60
dm_ima_measure_on_device_remove+0x3fc/0x6c0
dev_remove+0x123/0x1e0
ctl_ioctl+0x2a2/0x480
dm_ctl_ioctl+0xe/0x20
__x64_sys_ioctl+0xc7/0x110
do_syscall_64+0x72/0x390
entry_SYSCALL_64_after_hwframe+0x76/0x7e
To reproduce this issue, add a delay between freeing
md->ima.active_table.hash and setting it to NULL, using the following
steps:
dmsetup create mydevice --table "0 2097152 linear /dev/sda 0"
dmsetup suspend mydevice
dmsetup reload mydevice --table "0 2097152 linear /dev/sdb 0"
dmsetup resume mydevice &
dmsetup remove mydevice
The device_metadata also appears to have a similar issue.
Add spinlocks for hash and device_metadata to prevent concurrent
usage and release.
Fixes: 91ccbbac1747 ("dm ima: measure data on table load")
Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
---
drivers/md/dm-ima.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
drivers/md/dm-ima.h | 2 ++
2 files changed, 47 insertions(+)
diff --git a/drivers/md/dm-ima.c b/drivers/md/dm-ima.c
index b90f34259fbb..ff2ac54addb8 100644
--- a/drivers/md/dm-ima.c
+++ b/drivers/md/dm-ima.c
@@ -170,6 +170,10 @@ void dm_ima_reset_data(struct mapped_device *md)
{
memset(&(md->ima), 0, sizeof(md->ima));
md->ima.dm_version_str_len = strlen(DM_IMA_VERSION_STR);
+ spin_lock_init(&md->ima.active_table.device_metadata_lock);
+ spin_lock_init(&md->ima.inactive_table.device_metadata_lock);
+ spin_lock_init(&md->ima.active_table.hash_lock);
+ spin_lock_init(&md->ima.inactive_table.hash_lock);
}
/*
@@ -336,19 +340,24 @@ void dm_ima_measure_on_table_load(struct dm_table *table, unsigned int status_fl
for (i = 0; i < digest_size; i++)
snprintf((digest_buf + hash_alg_prefix_len + (i*2)), 3, "%02x", digest[i]);
+ spin_lock(&table->md->ima.inactive_table.hash_lock);
if (table->md->ima.active_table.hash != table->md->ima.inactive_table.hash)
kfree(table->md->ima.inactive_table.hash);
table->md->ima.inactive_table.hash = digest_buf;
table->md->ima.inactive_table.hash_len = strlen(digest_buf);
+ spin_unlock(&table->md->ima.inactive_table.hash_lock);
+
table->md->ima.inactive_table.num_targets = num_targets;
+ spin_lock(&table->md->ima.inactive_table.device_metadata_lock);
if (table->md->ima.active_table.device_metadata !=
table->md->ima.inactive_table.device_metadata)
kfree(table->md->ima.inactive_table.device_metadata);
table->md->ima.inactive_table.device_metadata = device_data_buf;
table->md->ima.inactive_table.device_metadata_len = device_data_buf_len;
+ spin_unlock(&table->md->ima.inactive_table.device_metadata_lock);
goto exit;
error:
@@ -388,18 +397,22 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap)
l += md->ima.dm_version_str_len;
if (swap) {
+ spin_lock(&md->ima.active_table.hash_lock);
if (md->ima.active_table.hash != md->ima.inactive_table.hash)
kfree(md->ima.active_table.hash);
md->ima.active_table.hash = NULL;
md->ima.active_table.hash_len = 0;
+ spin_unlock(&md->ima.active_table.hash_lock);
+ spin_lock(&md->ima.active_table.device_metadata_lock);
if (md->ima.active_table.device_metadata !=
md->ima.inactive_table.device_metadata)
kfree(md->ima.active_table.device_metadata);
md->ima.active_table.device_metadata = NULL;
md->ima.active_table.device_metadata_len = 0;
+ spin_unlock(&md->ima.active_table.device_metadata_lock);
md->ima.active_table.num_targets = 0;
if (md->ima.inactive_table.hash) {
@@ -429,6 +442,7 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap)
nodata = false;
}
+ spin_lock(&md->ima.active_table.hash_lock);
if (md->ima.active_table.hash) {
memcpy(device_table_data + l, active, active_len);
l += active_len;
@@ -442,6 +456,7 @@ void dm_ima_measure_on_device_resume(struct mapped_device *md, bool swap)
nodata = false;
}
+ spin_unlock(&md->ima.active_table.hash_lock);
if (nodata) {
r = dm_ima_alloc_and_copy_name_uuid(md, &dev_name, &dev_uuid, noio);
@@ -503,6 +518,7 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
memcpy(device_table_data + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
l += md->ima.dm_version_str_len;
+ spin_lock(&md->ima.active_table.device_metadata_lock);
if (md->ima.active_table.device_metadata) {
memcpy(device_table_data + l, device_active_str, device_active_len);
l += device_active_len;
@@ -513,7 +529,9 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
nodata = false;
}
+ spin_unlock(&md->ima.active_table.device_metadata_lock);
+ spin_lock(&md->ima.inactive_table.device_metadata_lock);
if (md->ima.inactive_table.device_metadata) {
memcpy(device_table_data + l, device_inactive_str, device_inactive_len);
l += device_inactive_len;
@@ -524,7 +542,9 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
nodata = false;
}
+ spin_unlock(&md->ima.inactive_table.device_metadata_lock);
+ spin_lock(&md->ima.active_table.hash_lock);
if (md->ima.active_table.hash) {
memcpy(device_table_data + l, active_table_str, active_table_len);
l += active_table_len;
@@ -538,7 +558,9 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
nodata = false;
}
+ spin_unlock(&md->ima.active_table.hash_lock);
+ spin_lock(&md->ima.inactive_table.hash_lock);
if (md->ima.inactive_table.hash) {
memcpy(device_table_data + l, inactive_table_str, inactive_table_len);
l += inactive_table_len;
@@ -552,6 +574,7 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
nodata = false;
}
+ spin_unlock(&md->ima.inactive_table.hash_lock);
/*
* In case both active and inactive tables, and corresponding
* device metadata is cleared/missing - record the name and uuid
@@ -582,17 +605,31 @@ void dm_ima_measure_on_device_remove(struct mapped_device *md, bool remove_all)
kfree(device_table_data);
kfree(capacity_str);
exit:
+ spin_lock(&md->ima.active_table.device_metadata_lock);
+ spin_lock(&md->ima.inactive_table.device_metadata_lock);
kfree(md->ima.active_table.device_metadata);
if (md->ima.active_table.device_metadata !=
md->ima.inactive_table.device_metadata)
kfree(md->ima.inactive_table.device_metadata);
+ md->ima.active_table.device_metadata = NULL;
+ md->ima.inactive_table.device_metadata = NULL;
+ spin_unlock(&md->ima.inactive_table.device_metadata_lock);
+ spin_unlock(&md->ima.active_table.device_metadata_lock);
+
+ spin_lock(&md->ima.active_table.hash_lock);
+ spin_lock(&md->ima.inactive_table.hash_lock);
kfree(md->ima.active_table.hash);
if (md->ima.active_table.hash != md->ima.inactive_table.hash)
kfree(md->ima.inactive_table.hash);
+ md->ima.active_table.hash = NULL;
+ md->ima.inactive_table.hash = NULL;
+ spin_unlock(&md->ima.inactive_table.hash_lock);
+ spin_unlock(&md->ima.active_table.hash_lock);
+
dm_ima_reset_data(md);
kfree(dev_name);
@@ -623,6 +660,8 @@ void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map)
memcpy(device_table_data + l, DM_IMA_VERSION_STR, md->ima.dm_version_str_len);
l += md->ima.dm_version_str_len;
+ spin_lock(&md->ima.inactive_table.hash_lock);
+ spin_lock(&md->ima.inactive_table.device_metadata_lock);
if (md->ima.inactive_table.device_metadata_len &&
md->ima.inactive_table.hash_len) {
memcpy(device_table_data + l, md->ima.inactive_table.device_metadata,
@@ -642,6 +681,8 @@ void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map)
nodata = false;
}
+ spin_unlock(&md->ima.inactive_table.device_metadata_lock);
+ spin_unlock(&md->ima.inactive_table.hash_lock);
if (nodata) {
if (dm_ima_alloc_and_copy_name_uuid(md, &dev_name, &dev_uuid, noio))
@@ -660,19 +701,23 @@ void dm_ima_measure_on_table_clear(struct mapped_device *md, bool new_map)
dm_ima_measure_data("dm_table_clear", device_table_data, l, noio);
if (new_map) {
+ spin_lock(&md->ima.inactive_table.hash_lock);
if (md->ima.inactive_table.hash &&
md->ima.inactive_table.hash != md->ima.active_table.hash)
kfree(md->ima.inactive_table.hash);
md->ima.inactive_table.hash = NULL;
md->ima.inactive_table.hash_len = 0;
+ spin_unlock(&md->ima.inactive_table.hash_lock);
+ spin_lock(&md->ima.inactive_table.device_metadata_lock);
if (md->ima.inactive_table.device_metadata &&
md->ima.inactive_table.device_metadata != md->ima.active_table.device_metadata)
kfree(md->ima.inactive_table.device_metadata);
md->ima.inactive_table.device_metadata = NULL;
md->ima.inactive_table.device_metadata_len = 0;
+ spin_unlock(&md->ima.inactive_table.device_metadata_lock);
md->ima.inactive_table.num_targets = 0;
if (md->ima.active_table.hash) {
diff --git a/drivers/md/dm-ima.h b/drivers/md/dm-ima.h
index 568870a1a145..a021056f4ab5 100644
--- a/drivers/md/dm-ima.h
+++ b/drivers/md/dm-ima.h
@@ -35,6 +35,7 @@ struct dm_ima_device_table_metadata {
* pairs delimited by a semicolon at the end of the list.
*/
char *device_metadata;
+ spinlock_t device_metadata_lock;
unsigned int device_metadata_len;
unsigned int num_targets;
@@ -43,6 +44,7 @@ struct dm_ima_device_table_metadata {
* attributes' key-value pairs from the active/inactive tables.
*/
char *hash;
+ spinlock_t hash_lock;
unsigned int hash_len;
};
--
2.46.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] dm: introduce spinlocks to prevent concurrent usage and release of device_metadata and hash
2025-07-17 9:43 ` Hou Tao
@ 2025-07-17 18:45 ` Mikulas Patocka
2025-07-18 1:28 ` Li Lingfeng
0 siblings, 1 reply; 4+ messages in thread
From: Mikulas Patocka @ 2025-07-17 18:45 UTC (permalink / raw)
To: Hou Tao
Cc: Li Lingfeng, dm-devel, agk, snitzer, tusharsu, colin.i.king,
linux-kernel, yi.zhang, yangerkun, yukuai3, chengzhihao1,
lilingfeng
On Thu, 17 Jul 2025, Hou Tao wrote:
> Hi,
>
> On 7/17/2025 10:07 PM, Li Lingfeng wrote:
> > There is a window between freeing md->ima.active_table.hash and setting
> > md->ima.active_table.hash to NULL in dm_ima_measure_on_device_resume().
> > If dm_ima_measure_on_device_remove() accesses md->ima.active_table.hash
> > concurrently during this window, it could lead to a double free or UAF,
> > as shown below:
>
> The UAF is a problem. However, I think the order of
> dm_ima_measure_on_device_remove() and dm_ima_measure_on_device_resume is
> bigger problem. Is it possible to order these two invocations properly
> by using suspend_lock ?
That's a good point. The patch just papers over the problem. It will fix
the UAF warning, but it won't fix the fact that
dm_ima_measure_on_device_resume can race with
dm_ima_measure_on_device_remove.
The real fix would be to make sure that dm_ima_measure_* calls are never
called concurrently on the same table - we should use existing device
mapper locks for that.
Mikukas
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] dm: introduce spinlocks to prevent concurrent usage and release of device_metadata and hash
2025-07-17 18:45 ` Mikulas Patocka
@ 2025-07-18 1:28 ` Li Lingfeng
0 siblings, 0 replies; 4+ messages in thread
From: Li Lingfeng @ 2025-07-18 1:28 UTC (permalink / raw)
To: Mikulas Patocka, Hou Tao
Cc: dm-devel, agk, snitzer, tusharsu, colin.i.king, linux-kernel,
yi.zhang, yangerkun, yukuai3, chengzhihao1, lilingfeng
在 2025/7/18 2:45, Mikulas Patocka 写道:
>
> On Thu, 17 Jul 2025, Hou Tao wrote:
>
>> Hi,
>>
>> On 7/17/2025 10:07 PM, Li Lingfeng wrote:
>>> There is a window between freeing md->ima.active_table.hash and setting
>>> md->ima.active_table.hash to NULL in dm_ima_measure_on_device_resume().
>>> If dm_ima_measure_on_device_remove() accesses md->ima.active_table.hash
>>> concurrently during this window, it could lead to a double free or UAF,
>>> as shown below:
>> The UAF is a problem. However, I think the order of
>> dm_ima_measure_on_device_remove() and dm_ima_measure_on_device_resume is
>> bigger problem. Is it possible to order these two invocations properly
>> by using suspend_lock ?
> That's a good point. The patch just papers over the problem. It will fix
> the UAF warning, but it won't fix the fact that
> dm_ima_measure_on_device_resume can race with
> dm_ima_measure_on_device_remove.
>
> The real fix would be to make sure that dm_ima_measure_* calls are never
> called concurrently on the same table - we should use existing device
> mapper locks for that.
>
> Mikukas
>
Thanks for your advice, I will send a new patch soon.
Thanks,
Lingfeng
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-07-18 1:28 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-17 14:07 [PATCH] dm: introduce spinlocks to prevent concurrent usage and release of device_metadata and hash Li Lingfeng
2025-07-17 9:43 ` Hou Tao
2025-07-17 18:45 ` Mikulas Patocka
2025-07-18 1:28 ` Li Lingfeng
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).