diff mbox series

zram: fix NULL pointer in comp_algorithm_show()

Message ID 20241108100147.3776123-1-liushixin2@huawei.com (mailing list archive)
State New
Headers show
Series zram: fix NULL pointer in comp_algorithm_show() | expand

Commit Message

Liu Shixin Nov. 8, 2024, 10:01 a.m. UTC 
LTP reported a NULL pointer dereference as followed:

 CPU: 7 UID: 0 PID: 5995 Comm: cat Kdump: loaded Not tainted 6.12.0-rc6+ #3
 Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
 pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
 pc : __pi_strcmp+0x24/0x140
 lr : zcomp_available_show+0x60/0x100 [zram]
 sp : ffff800088b93b90
 x29: ffff800088b93b90 x28: 0000000000000001 x27: 0000000000400cc0
 x26: 0000000000000ffe x25: ffff80007b3e2388 x24: 0000000000000000
 x23: ffff80007b3e2390 x22: ffff0004041a9000 x21: ffff80007b3e2900
 x20: 0000000000000000 x19: 0000000000000000 x18: 0000000000000000
 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
 x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
 x11: 0000000000000000 x10: ffff80007b3e2900 x9 : ffff80007b3cb280
 x8 : 0101010101010101 x7 : 0000000000000000 x6 : 0000000000000000
 x5 : 0000000000000040 x4 : 0000000000000000 x3 : 00656c722d6f7a6c
 x2 : 0000000000000000 x1 : ffff80007b3e2900 x0 : 0000000000000000
 Call trace:
  __pi_strcmp+0x24/0x140
  comp_algorithm_show+0x40/0x70 [zram]
  dev_attr_show+0x28/0x80
  sysfs_kf_seq_show+0x90/0x140
  kernfs_seq_show+0x34/0x48
  seq_read_iter+0x1d4/0x4e8
  kernfs_fop_read_iter+0x40/0x58
  new_sync_read+0x9c/0x168
  vfs_read+0x1a8/0x1f8
  ksys_read+0x74/0x108
  __arm64_sys_read+0x24/0x38
  invoke_syscall+0x50/0x120
  el0_svc_common.constprop.0+0xc8/0xf0
  do_el0_svc+0x24/0x38
  el0_svc+0x38/0x138
  el0t_64_sync_handler+0xc0/0xc8
  el0t_64_sync+0x188/0x190

The zram->comp_algs[ZRAM_PRIMARY_COMP] can be NULL in zram_add() if
comp_algorithm_set() has not been called. User can access the zram device
by sysfs after device_add_disk(), so there is a time window to trigger
the NULL pointer dereference. Move it ahead device_add_disk() to make sure
when user can access the zram device, it is ready. comp_algorithm_set() is
protected by zram->init_lock in other places and no such problem.

Fixes: 7ac07a26dea7 ("zram: preparation for multi-zcomp support")
Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 drivers/block/zram/zram_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Sergey Senozhatsky Nov. 9, 2024, 1:12 a.m. UTC | #1 
On (24/11/08 18:01), Liu Shixin wrote:
> LTP reported a NULL pointer dereference as followed:
> 
>  CPU: 7 UID: 0 PID: 5995 Comm: cat Kdump: loaded Not tainted 6.12.0-rc6+ #3
>  Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
>  pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : __pi_strcmp+0x24/0x140
>  lr : zcomp_available_show+0x60/0x100 [zram]
>  sp : ffff800088b93b90
>  x29: ffff800088b93b90 x28: 0000000000000001 x27: 0000000000400cc0
>  x26: 0000000000000ffe x25: ffff80007b3e2388 x24: 0000000000000000
>  x23: ffff80007b3e2390 x22: ffff0004041a9000 x21: ffff80007b3e2900
>  x20: 0000000000000000 x19: 0000000000000000 x18: 0000000000000000
>  x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
>  x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
>  x11: 0000000000000000 x10: ffff80007b3e2900 x9 : ffff80007b3cb280
>  x8 : 0101010101010101 x7 : 0000000000000000 x6 : 0000000000000000
>  x5 : 0000000000000040 x4 : 0000000000000000 x3 : 00656c722d6f7a6c
>  x2 : 0000000000000000 x1 : ffff80007b3e2900 x0 : 0000000000000000
>  Call trace:
>   __pi_strcmp+0x24/0x140
>   comp_algorithm_show+0x40/0x70 [zram]
>   dev_attr_show+0x28/0x80
>   sysfs_kf_seq_show+0x90/0x140
>   kernfs_seq_show+0x34/0x48
>   seq_read_iter+0x1d4/0x4e8
>   kernfs_fop_read_iter+0x40/0x58
>   new_sync_read+0x9c/0x168
>   vfs_read+0x1a8/0x1f8
>   ksys_read+0x74/0x108
>   __arm64_sys_read+0x24/0x38
>   invoke_syscall+0x50/0x120
>   el0_svc_common.constprop.0+0xc8/0xf0
>   do_el0_svc+0x24/0x38
>   el0_svc+0x38/0x138
>   el0t_64_sync_handler+0xc0/0xc8
>   el0t_64_sync+0x188/0x190

The explanation below is more than enough, I think this stack trace
doesn't really show anything new or interesting.

> The zram->comp_algs[ZRAM_PRIMARY_COMP] can be NULL in zram_add() if
> comp_algorithm_set() has not been called. User can access the zram device
> by sysfs after device_add_disk(), so there is a time window to trigger
> the NULL pointer dereference. Move it ahead device_add_disk() to make sure
> when user can access the zram device, it is ready. comp_algorithm_set() is
> protected by zram->init_lock in other places and no such problem.
> 
> Fixes: 7ac07a26dea7 ("zram: preparation for multi-zcomp support")

So I think this fixes something much older, probably around e46b8a030d76d
time (2014).

> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> ---

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
Liu Shixin Nov. 9, 2024, 3:23 a.m. UTC | #2 
On 2024/11/9 9:12, Sergey Senozhatsky wrote:
> On (24/11/08 18:01), Liu Shixin wrote:
>> LTP reported a NULL pointer dereference as followed:
>>
>>  CPU: 7 UID: 0 PID: 5995 Comm: cat Kdump: loaded Not tainted 6.12.0-rc6+ #3
>>  Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
>>  pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>  pc : __pi_strcmp+0x24/0x140
>>  lr : zcomp_available_show+0x60/0x100 [zram]
>>  sp : ffff800088b93b90
>>  x29: ffff800088b93b90 x28: 0000000000000001 x27: 0000000000400cc0
>>  x26: 0000000000000ffe x25: ffff80007b3e2388 x24: 0000000000000000
>>  x23: ffff80007b3e2390 x22: ffff0004041a9000 x21: ffff80007b3e2900
>>  x20: 0000000000000000 x19: 0000000000000000 x18: 0000000000000000
>>  x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
>>  x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
>>  x11: 0000000000000000 x10: ffff80007b3e2900 x9 : ffff80007b3cb280
>>  x8 : 0101010101010101 x7 : 0000000000000000 x6 : 0000000000000000
>>  x5 : 0000000000000040 x4 : 0000000000000000 x3 : 00656c722d6f7a6c
>>  x2 : 0000000000000000 x1 : ffff80007b3e2900 x0 : 0000000000000000
>>  Call trace:
>>   __pi_strcmp+0x24/0x140
>>   comp_algorithm_show+0x40/0x70 [zram]
>>   dev_attr_show+0x28/0x80
>>   sysfs_kf_seq_show+0x90/0x140
>>   kernfs_seq_show+0x34/0x48
>>   seq_read_iter+0x1d4/0x4e8
>>   kernfs_fop_read_iter+0x40/0x58
>>   new_sync_read+0x9c/0x168
>>   vfs_read+0x1a8/0x1f8
>>   ksys_read+0x74/0x108
>>   __arm64_sys_read+0x24/0x38
>>   invoke_syscall+0x50/0x120
>>   el0_svc_common.constprop.0+0xc8/0xf0
>>   do_el0_svc+0x24/0x38
>>   el0_svc+0x38/0x138
>>   el0t_64_sync_handler+0xc0/0xc8
>>   el0t_64_sync+0x188/0x190
> The explanation below is more than enough, I think this stack trace
> doesn't really show anything new or interesting.
>
>> The zram->comp_algs[ZRAM_PRIMARY_COMP] can be NULL in zram_add() if
>> comp_algorithm_set() has not been called. User can access the zram device
>> by sysfs after device_add_disk(), so there is a time window to trigger
>> the NULL pointer dereference. Move it ahead device_add_disk() to make sure
>> when user can access the zram device, it is ready. comp_algorithm_set() is
>> protected by zram->init_lock in other places and no such problem.
>>
>> Fixes: 7ac07a26dea7 ("zram: preparation for multi-zcomp support")
> So I think this fixes something much older, probably around e46b8a030d76d
> time (2014).

The NULL pointer is introduced when zram->compressor is changed to zram->comp_algs in
7ac07a26dea, where the former is an array of chars and the latter is an array of pointers.
Before that, if we cat comp_algorithm int the time window, we will get a message with no
algorithm selected like this:

    cat /sys/block/zram1/comp_algorithm
    lzo lz4

>
>> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
>> ---
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> .
>

Thanks,
.
Sergey Senozhatsky Nov. 9, 2024, 3:42 a.m. UTC | #3 
On (24/11/09 11:23), Liu Shixin wrote:
> >> Fixes: 7ac07a26dea7 ("zram: preparation for multi-zcomp support")
> > So I think this fixes something much older, probably around e46b8a030d76d
> > time (2014).
> 
> The NULL pointer is introduced when zram->compressor is changed to zram->comp_algs in
> 7ac07a26dea, where the former is an array of chars and the latter is an array of pointers.
> Before that, if we cat comp_algorithm int the time window, we will get a message with no
> algorithm selected like this:

Good point, you are right.
Jens Axboe Nov. 10, 2024, 3:07 a.m. UTC | #4 
On Fri, 08 Nov 2024 18:01:47 +0800, Liu Shixin wrote:
> LTP reported a NULL pointer dereference as followed:
> 
>  CPU: 7 UID: 0 PID: 5995 Comm: cat Kdump: loaded Not tainted 6.12.0-rc6+ #3
>  Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
>  pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>  pc : __pi_strcmp+0x24/0x140
>  lr : zcomp_available_show+0x60/0x100 [zram]
>  sp : ffff800088b93b90
>  x29: ffff800088b93b90 x28: 0000000000000001 x27: 0000000000400cc0
>  x26: 0000000000000ffe x25: ffff80007b3e2388 x24: 0000000000000000
>  x23: ffff80007b3e2390 x22: ffff0004041a9000 x21: ffff80007b3e2900
>  x20: 0000000000000000 x19: 0000000000000000 x18: 0000000000000000
>  x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
>  x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
>  x11: 0000000000000000 x10: ffff80007b3e2900 x9 : ffff80007b3cb280
>  x8 : 0101010101010101 x7 : 0000000000000000 x6 : 0000000000000000
>  x5 : 0000000000000040 x4 : 0000000000000000 x3 : 00656c722d6f7a6c
>  x2 : 0000000000000000 x1 : ffff80007b3e2900 x0 : 0000000000000000
>  Call trace:
>   __pi_strcmp+0x24/0x140
>   comp_algorithm_show+0x40/0x70 [zram]
>   dev_attr_show+0x28/0x80
>   sysfs_kf_seq_show+0x90/0x140
>   kernfs_seq_show+0x34/0x48
>   seq_read_iter+0x1d4/0x4e8
>   kernfs_fop_read_iter+0x40/0x58
>   new_sync_read+0x9c/0x168
>   vfs_read+0x1a8/0x1f8
>   ksys_read+0x74/0x108
>   __arm64_sys_read+0x24/0x38
>   invoke_syscall+0x50/0x120
>   el0_svc_common.constprop.0+0xc8/0xf0
>   do_el0_svc+0x24/0x38
>   el0_svc+0x38/0x138
>   el0t_64_sync_handler+0xc0/0xc8
>   el0t_64_sync+0x188/0x190
> 
> [...]

Applied, thanks!

[1/1] zram: fix NULL pointer in comp_algorithm_show()
      commit: 5fcfcd51ea1c1309a673d45f12698dec53ffbd9b

Best regards,
diff mbox series

Patch

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index ad9c9bc3ccfc..5223a03cb10e 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -2381,6 +2381,8 @@  static int zram_add(void)
 	zram->disk->fops = &zram_devops;
 	zram->disk->private_data = zram;
 	snprintf(zram->disk->disk_name, 16, "zram%d", device_id);
+	zram_comp_params_reset(zram);
+	comp_algorithm_set(zram, ZRAM_PRIMARY_COMP, default_compressor);
 
 	/* Actual capacity set using sysfs (/sys/block/zram<id>/disksize */
 	set_capacity(zram->disk, 0);
@@ -2388,9 +2390,6 @@  static int zram_add(void)
 	if (ret)
 		goto out_cleanup_disk;
 
-	zram_comp_params_reset(zram);
-	comp_algorithm_set(zram, ZRAM_PRIMARY_COMP, default_compressor);
-
 	zram_debugfs_register(zram);
 	pr_info("Added device: %s\n", zram->disk->disk_name);
 	return device_id;