[PATCH v2 6.1.y 0/3] Backport to fix CVE-2024-36478

[PATCH v2 6.1.y 0/3] Backport to fix CVE-2024-36478

[Xiangyu Chen]

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@gregkh/

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(-)

-- 
2.43.0

[PATCH v2 6.1.y 1/3] null_blk: Remove usage of the deprecated ida_simple_xx() API

[Xiangyu Chen]

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.1705222845.git.christophe.jaillet@wanadoo.fr
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;
-- 
2.43.0

[PATCH v2 6.1.y 2/3] null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'

[Xiangyu Chen]

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+xT7AP7TPnEjStuwZA@mail.gmail.com/
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;
-- 
2.43.0

[PATCH v2 6.1.y 3/3] null_blk: Fix return value of nullb_device_power_store()

[Xiangyu Chen]

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;
-- 
2.43.0

Re: [PATCH v2 6.1.y 0/3] Backport to fix CVE-2024-36478

[Greg KH]

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@gregkh/
> 
> 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

Re: [PATCH v2 6.1.y 2/3] null_blk: fix null-ptr-dereference while configuring 'power' and 'submit_queues'

[Sasha Levin]

[ 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   |

Re: [PATCH v2 6.1.y 3/3] null_blk: Fix return value of nullb_device_power_store()

[Sasha Levin]

[ 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   |

Re: [PATCH v2 6.1.y 1/3] null_blk: Remove usage of the deprecated ida_simple_xx() API

[Sasha Levin]

[ 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.1705222845.git.christophe.jaillet@wanadoo.fr
 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   |