From: Xiangyu Chen xiangyu.chen@windriver.com
Backport to fix CVE-2024-36478
https://lore.kernel.org/linux-cve-announce/2024062136-CVE-2024-36478-d249@gr...
The CVE fix is "null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'"
This required 2 extra commit to make sure the picks are clean: null_blk: Remove usage of the deprecated ida_simple_xx() API null_blk: Fix return value of nullb_device_power_store()
Changes: V1 -> V2 Added the extra commit Fix return value of nullb_device_power_store()
Christophe JAILLET (1): null_blk: Remove usage of the deprecated ida_simple_xx() API
Damien Le Moal (1): null_blk: Fix return value of nullb_device_power_store()
Yu Kuai (1): null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'
drivers/block/null_blk/main.c | 45 ++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 16 deletions(-)
From: Christophe JAILLET christophe.jaillet@wanadoo.fr
[ Upstream commit 95931a245b44ee04f3359ec432e73614d44d8b38 ]
ida_alloc() and ida_free() should be preferred to the deprecated ida_simple_get() and ida_simple_remove().
This is less verbose.
Signed-off-by: Christophe JAILLET christophe.jaillet@wanadoo.fr Link: https://lore.kernel.org/r/bf257b1078475a415cdc3344c6a750842946e367.170522284... Signed-off-by: Jens Axboe axboe@kernel.dk Signed-off-by: Xiangyu Chen xiangyu.chen@windriver.com --- drivers/block/null_blk/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index 4d78b5583dc6..f58778b57375 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -1764,7 +1764,7 @@ static void null_del_dev(struct nullb *nullb)
dev = nullb->dev;
- ida_simple_remove(&nullb_indexes, nullb->index); + ida_free(&nullb_indexes, nullb->index);
list_del_init(&nullb->list);
@@ -2103,7 +2103,7 @@ static int null_add_dev(struct nullb_device *dev) blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, nullb->q);
mutex_lock(&lock); - rv = ida_simple_get(&nullb_indexes, 0, 0, GFP_KERNEL); + rv = ida_alloc(&nullb_indexes, GFP_KERNEL); if (rv < 0) { mutex_unlock(&lock); goto out_cleanup_zone;
[ Sasha's backport helper bot ]
Hi,
The upstream commit SHA1 provided is correct: 95931a245b44ee04f3359ec432e73614d44d8b38
WARNING: Author mismatch between patch and upstream commit: Backport author: Xiangyu Chen xiangyu.chen@eng.windriver.com Commit author: Christophe JAILLET christophe.jaillet@wanadoo.fr
Status in newer kernel trees: 6.11.y | Present (exact SHA1) 6.6.y | Present (different SHA1: b2b02202f87d) 6.1.y | Not found
Note: The patch differs from the upstream commit: --- --- - 2024-11-20 07:40:44.222155724 -0500 +++ /tmp/tmp.aIhRvR57Io 2024-11-20 07:40:44.215907652 -0500 @@ -1,3 +1,5 @@ +[ Upstream commit 95931a245b44ee04f3359ec432e73614d44d8b38 ] + ida_alloc() and ida_free() should be preferred to the deprecated ida_simple_get() and ida_simple_remove().
@@ -6,15 +8,16 @@ Signed-off-by: Christophe JAILLET christophe.jaillet@wanadoo.fr Link: https://lore.kernel.org/r/bf257b1078475a415cdc3344c6a750842946e367.170522284... Signed-off-by: Jens Axboe axboe@kernel.dk +Signed-off-by: Xiangyu Chen xiangyu.chen@windriver.com --- drivers/block/null_blk/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c -index 9f7695f00c2db..36755f263e8ec 100644 +index 4d78b5583dc6..f58778b57375 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c -@@ -1840,7 +1840,7 @@ static void null_del_dev(struct nullb *nullb) +@@ -1764,7 +1764,7 @@ static void null_del_dev(struct nullb *nullb)
dev = nullb->dev;
@@ -23,8 +26,8 @@
list_del_init(&nullb->list);
-@@ -2174,7 +2174,7 @@ static int null_add_dev(struct nullb_device *dev) - blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q); +@@ -2103,7 +2103,7 @@ static int null_add_dev(struct nullb_device *dev) + blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, nullb->q);
mutex_lock(&lock); - rv = ida_simple_get(&nullb_indexes, 0, 0, GFP_KERNEL); @@ -32,3 +35,6 @@ if (rv < 0) { mutex_unlock(&lock); goto out_cleanup_zone; +-- +2.43.0 + ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.1.y | Success | Success |
From: Yu Kuai yukuai3@huawei.com
[ Upstream commit a2db328b0839312c169eb42746ec46fc1ab53ed2 ]
Writing 'power' and 'submit_queues' concurrently will trigger kernel panic:
Test script:
modprobe null_blk nr_devices=0 mkdir -p /sys/kernel/config/nullb/nullb0 while true; do echo 1 > submit_queues; echo 4 > submit_queues; done & while true; do echo 1 > power; echo 0 > power; done
Test result:
BUG: kernel NULL pointer dereference, address: 0000000000000148 Oops: 0000 [#1] PREEMPT SMP RIP: 0010:__lock_acquire+0x41d/0x28f0 Call Trace: <TASK> lock_acquire+0x121/0x450 down_write+0x5f/0x1d0 simple_recursive_removal+0x12f/0x5c0 blk_mq_debugfs_unregister_hctxs+0x7c/0x100 blk_mq_update_nr_hw_queues+0x4a3/0x720 nullb_update_nr_hw_queues+0x71/0xf0 [null_blk] nullb_device_submit_queues_store+0x79/0xf0 [null_blk] configfs_write_iter+0x119/0x1e0 vfs_write+0x326/0x730 ksys_write+0x74/0x150
This is because del_gendisk() can concurrent with blk_mq_update_nr_hw_queues():
nullb_device_power_store nullb_apply_submit_queues null_del_dev del_gendisk nullb_update_nr_hw_queues if (!dev->nullb) // still set while gendisk is deleted return 0 blk_mq_update_nr_hw_queues dev->nullb = NULL
Fix this problem by resuing the global mutex to protect nullb_device_power_store() and nullb_update_nr_hw_queues() from configfs.
Fixes: 45919fbfe1c4 ("null_blk: Enable modifying 'submit_queues' after an instance has been configured") Reported-and-tested-by: Yi Zhang yi.zhang@redhat.com Closes: https://lore.kernel.org/all/CAHj4cs9LgsHLnjg8z06LQ3Pr5cax-+Ps+xT7AP7TPnEjStu... Signed-off-by: Yu Kuai yukuai3@huawei.com Reviewed-by: Zhu Yanjun yanjun.zhu@linux.dev Link: https://lore.kernel.org/r/20240523153934.1937851-1-yukuai1@huaweicloud.com Signed-off-by: Jens Axboe axboe@kernel.dk Signed-off-by: Sasha Levin sashal@kernel.org Signed-off-by: Xiangyu Chen xiangyu.chen@windriver.com --- drivers/block/null_blk/main.c | 40 +++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index f58778b57375..e838eed4aacf 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -392,13 +392,25 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev, static int nullb_apply_submit_queues(struct nullb_device *dev, unsigned int submit_queues) { - return nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues); + int ret; + + mutex_lock(&lock); + ret = nullb_update_nr_hw_queues(dev, submit_queues, dev->poll_queues); + mutex_unlock(&lock); + + return ret; }
static int nullb_apply_poll_queues(struct nullb_device *dev, unsigned int poll_queues) { - return nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues); + int ret; + + mutex_lock(&lock); + ret = nullb_update_nr_hw_queues(dev, dev->submit_queues, poll_queues); + mutex_unlock(&lock); + + return ret; }
NULLB_DEVICE_ATTR(size, ulong, NULL); @@ -444,28 +456,31 @@ static ssize_t nullb_device_power_store(struct config_item *item, if (ret < 0) return ret;
+ ret = count; + mutex_lock(&lock); if (!dev->power && newp) { if (test_and_set_bit(NULLB_DEV_FL_UP, &dev->flags)) - return count; + goto out; + ret = null_add_dev(dev); if (ret) { clear_bit(NULLB_DEV_FL_UP, &dev->flags); - return ret; + goto out; }
set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); dev->power = newp; } else if (dev->power && !newp) { if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) { - mutex_lock(&lock); dev->power = newp; null_del_dev(dev->nullb); - mutex_unlock(&lock); } clear_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); }
- return count; +out: + mutex_unlock(&lock); + return ret; }
CONFIGFS_ATTR(nullb_device_, power); @@ -2102,15 +2117,12 @@ static int null_add_dev(struct nullb_device *dev) blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q); blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, nullb->q);
- mutex_lock(&lock); rv = ida_alloc(&nullb_indexes, GFP_KERNEL); - if (rv < 0) { - mutex_unlock(&lock); + if (rv < 0) goto out_cleanup_zone; - } + nullb->index = rv; dev->index = rv; - mutex_unlock(&lock);
blk_queue_logical_block_size(nullb->q, dev->blocksize); blk_queue_physical_block_size(nullb->q, dev->blocksize); @@ -2134,9 +2146,7 @@ static int null_add_dev(struct nullb_device *dev) if (rv) goto out_ida_free;
- mutex_lock(&lock); list_add_tail(&nullb->list, &nullb_list); - mutex_unlock(&lock);
pr_info("disk %s created\n", nullb->disk_name);
@@ -2185,7 +2195,9 @@ static int null_create_dev(void) if (!dev) return -ENOMEM;
+ mutex_lock(&lock); ret = null_add_dev(dev); + mutex_unlock(&lock); if (ret) { null_free_dev(dev); return ret;
[ Sasha's backport helper bot ]
Hi,
The upstream commit SHA1 provided is correct: a2db328b0839312c169eb42746ec46fc1ab53ed2
WARNING: Author mismatch between patch and upstream commit: Backport author: Xiangyu Chen xiangyu.chen@eng.windriver.com Commit author: Yu Kuai yukuai3@huawei.com
Status in newer kernel trees: 6.11.y | Present (exact SHA1) 6.6.y | Present (different SHA1: aaadb755f2d6) 6.1.y | Not found
Note: The patch differs from the upstream commit: --- --- - 2024-11-20 07:45:51.467026214 -0500 +++ /tmp/tmp.d9kjHboEiG 2024-11-20 07:45:51.459443624 -0500 @@ -1,3 +1,5 @@ +[ Upstream commit a2db328b0839312c169eb42746ec46fc1ab53ed2 ] + Writing 'power' and 'submit_queues' concurrently will trigger kernel panic:
@@ -49,15 +51,17 @@ Reviewed-by: Zhu Yanjun yanjun.zhu@linux.dev Link: https://lore.kernel.org/r/20240523153934.1937851-1-yukuai1@huaweicloud.com Signed-off-by: Jens Axboe axboe@kernel.dk +Signed-off-by: Sasha Levin sashal@kernel.org +Signed-off-by: Xiangyu Chen xiangyu.chen@windriver.com --- drivers/block/null_blk/main.c | 40 +++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c -index 5d56ad4ce01a1..eb023d2673693 100644 +index f58778b57375..e838eed4aacf 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c -@@ -413,13 +413,25 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev, +@@ -392,13 +392,25 @@ static int nullb_update_nr_hw_queues(struct nullb_device *dev, static int nullb_apply_submit_queues(struct nullb_device *dev, unsigned int submit_queues) { @@ -85,7 +89,7 @@ }
NULLB_DEVICE_ATTR(size, ulong, NULL); -@@ -468,28 +480,31 @@ static ssize_t nullb_device_power_store(struct config_item *item, +@@ -444,28 +456,31 @@ static ssize_t nullb_device_power_store(struct config_item *item, if (ret < 0) return ret;
@@ -122,25 +126,25 @@ }
CONFIGFS_ATTR(nullb_device_, power); -@@ -1932,15 +1947,12 @@ static int null_add_dev(struct nullb_device *dev) - nullb->q->queuedata = nullb; +@@ -2102,15 +2117,12 @@ static int null_add_dev(struct nullb_device *dev) blk_queue_flag_set(QUEUE_FLAG_NONROT, nullb->q); + blk_queue_flag_clear(QUEUE_FLAG_ADD_RANDOM, nullb->q);
- mutex_lock(&lock); rv = ida_alloc(&nullb_indexes, GFP_KERNEL); - if (rv < 0) { - mutex_unlock(&lock); + if (rv < 0) - goto out_cleanup_disk; + goto out_cleanup_zone; - } + nullb->index = rv; dev->index = rv; - mutex_unlock(&lock);
- if (config_item_name(&dev->group.cg_item)) { - /* Use configfs dir name as the device name */ -@@ -1969,9 +1981,7 @@ static int null_add_dev(struct nullb_device *dev) + blk_queue_logical_block_size(nullb->q, dev->blocksize); + blk_queue_physical_block_size(nullb->q, dev->blocksize); +@@ -2134,9 +2146,7 @@ static int null_add_dev(struct nullb_device *dev) if (rv) goto out_ida_free;
@@ -150,7 +154,7 @@
pr_info("disk %s created\n", nullb->disk_name);
-@@ -2020,7 +2030,9 @@ static int null_create_dev(void) +@@ -2185,7 +2195,9 @@ static int null_create_dev(void) if (!dev) return -ENOMEM;
@@ -160,3 +164,6 @@ if (ret) { null_free_dev(dev); return ret; +-- +2.43.0 + ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.1.y | Success | Success |
From: Damien Le Moal dlemoal@kernel.org
commit d9ff882b54f99f96787fa3df7cd938966843c418 upstream.
When powering on a null_blk device that is not already on, the return value ret that is initialized to be count is reused to check the return value of null_add_dev(), leading to nullb_device_power_store() to return null_add_dev() return value (0 on success) instead of "count". So make sure to set ret to be equal to count when there are no errors.
Fixes: a2db328b0839 ("null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'") Signed-off-by: Damien Le Moal dlemoal@kernel.org Reviewed-by: Yu Kuai yukuai3@huawei.com Reviewed-by: Kanchan Joshi joshi.k@samsung.com Link: https://lore.kernel.org/r/20240527043445.235267-1-dlemoal@kernel.org Signed-off-by: Jens Axboe axboe@kernel.dk Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org Signed-off-by: Xiangyu Chen xiangyu.chen@windriver.com --- drivers/block/null_blk/main.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c index e838eed4aacf..e66cace433cb 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c @@ -470,6 +470,7 @@ static ssize_t nullb_device_power_store(struct config_item *item,
set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); dev->power = newp; + ret = count; } else if (dev->power && !newp) { if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) { dev->power = newp;
[ Sasha's backport helper bot ]
Hi,
The upstream commit SHA1 provided is correct: d9ff882b54f99f96787fa3df7cd938966843c418
WARNING: Author mismatch between patch and upstream commit: Backport author: Xiangyu Chen xiangyu.chen@eng.windriver.com Commit author: Damien Le Moal dlemoal@kernel.org
Status in newer kernel trees: 6.11.y | Present (exact SHA1) 6.6.y | Present (different SHA1: e0aba0c6d521) 6.1.y | Not found
Note: The patch differs from the upstream commit: --- --- - 2024-11-20 07:50:08.726024900 -0500 +++ /tmp/tmp.mzeJOcYvZi 2024-11-20 07:50:08.723707071 -0500 @@ -1,3 +1,5 @@ +commit d9ff882b54f99f96787fa3df7cd938966843c418 upstream. + When powering on a null_blk device that is not already on, the return value ret that is initialized to be count is reused to check the return value of null_add_dev(), leading to nullb_device_power_store() to return @@ -10,15 +12,17 @@ Reviewed-by: Kanchan Joshi joshi.k@samsung.com Link: https://lore.kernel.org/r/20240527043445.235267-1-dlemoal@kernel.org Signed-off-by: Jens Axboe axboe@kernel.dk +Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org +Signed-off-by: Xiangyu Chen xiangyu.chen@windriver.com --- drivers/block/null_blk/main.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c -index eb023d2673693..631dca2e4e844 100644 +index e838eed4aacf..e66cace433cb 100644 --- a/drivers/block/null_blk/main.c +++ b/drivers/block/null_blk/main.c -@@ -494,6 +494,7 @@ static ssize_t nullb_device_power_store(struct config_item *item, +@@ -470,6 +470,7 @@ static ssize_t nullb_device_power_store(struct config_item *item,
set_bit(NULLB_DEV_FL_CONFIGURED, &dev->flags); dev->power = newp; @@ -26,3 +30,6 @@ } else if (dev->power && !newp) { if (test_and_clear_bit(NULLB_DEV_FL_UP, &dev->flags)) { dev->power = newp; +-- +2.43.0 + ---
Results of testing on various branches:
| Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.1.y | Success | Success |
On Wed, Nov 20, 2024 at 11:28:38AM +0800, Xiangyu Chen wrote:
From: Xiangyu Chen xiangyu.chen@windriver.com
Backport to fix CVE-2024-36478
https://lore.kernel.org/linux-cve-announce/2024062136-CVE-2024-36478-d249@gr...
The CVE fix is "null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'"
This required 2 extra commit to make sure the picks are clean: null_blk: Remove usage of the deprecated ida_simple_xx() API null_blk: Fix return value of nullb_device_power_store()
Changes: V1 -> V2 Added the extra commit Fix return value of nullb_device_power_store()
Now queued up, thanks.
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Sasha Levin -
Xiangyu Chen