[PATCH] scsi: scsi_debug: fix return checks for kcalloc

[PATCH] scsi: scsi_debug: fix return checks for kcalloc

[George Kennedy]

Change return checks from kcalloc() to now check for NULL and
ZERO_SIZE_PTR using the ZERO_OR_NULL_PTR macro or the following
crash can occur if ZERO_SIZE_PTR indicator is returned.

BUG: KASAN: null-ptr-deref in memcpy include/linux/fortify-string.h:191 [inline]
BUG: KASAN: null-ptr-deref in sg_copy_buffer+0x138/0x240 lib/scatterlist.c:974
Write of size 4 at addr 0000000000000010 by task syz-executor.1/22789

CPU: 1 PID: 22789 Comm: syz-executor.1 Not tainted 5.15.0-syzk #1
Hardware name: Red Hat KVM, BIOS 1.13.0-2
Call Trace:
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x89/0xb5 lib/dump_stack.c:106
 __kasan_report mm/kasan/report.c:446 [inline]
 kasan_report.cold.14+0x112/0x117 mm/kasan/report.c:459
 check_region_inline mm/kasan/generic.c:183 [inline]
 kasan_check_range+0x1a3/0x210 mm/kasan/generic.c:189
 memcpy+0x3b/0x60 mm/kasan/shadow.c:66
 memcpy include/linux/fortify-string.h:191 [inline]
 sg_copy_buffer+0x138/0x240 lib/scatterlist.c:974
 do_dout_fetch drivers/scsi/scsi_debug.c:2954 [inline]
 do_dout_fetch drivers/scsi/scsi_debug.c:2946 [inline]
 resp_verify+0x49e/0x930 drivers/scsi/scsi_debug.c:4276
 schedule_resp+0x4d8/0x1a70 drivers/scsi/scsi_debug.c:5478
 scsi_debug_queuecommand+0x8c9/0x1ec0 drivers/scsi/scsi_debug.c:7533
 scsi_dispatch_cmd drivers/scsi/scsi_lib.c:1520 [inline]
 scsi_queue_rq+0x16b0/0x2d40 drivers/scsi/scsi_lib.c:1699
 blk_mq_dispatch_rq_list+0xb9b/0x2700 block/blk-mq.c:1639
 __blk_mq_sched_dispatch_requests+0x28f/0x590 block/blk-mq-sched.c:325
 blk_mq_sched_dispatch_requests+0x105/0x190 block/blk-mq-sched.c:358
 __blk_mq_run_hw_queue+0xe5/0x150 block/blk-mq.c:1761
 __blk_mq_delay_run_hw_queue+0x4f8/0x5c0 block/blk-mq.c:1838
 blk_mq_run_hw_queue+0x18d/0x350 block/blk-mq.c:1891
 blk_mq_sched_insert_request+0x3db/0x4e0 block/blk-mq-sched.c:474
 blk_execute_rq_nowait+0x16b/0x1c0 block/blk-exec.c:62
 blk_execute_rq+0xdb/0x360 block/blk-exec.c:102
 sg_scsi_ioctl drivers/scsi/scsi_ioctl.c:621 [inline]
 scsi_ioctl+0x8bb/0x15c0 drivers/scsi/scsi_ioctl.c:930
 sg_ioctl_common+0x172d/0x2710 drivers/scsi/sg.c:1112
 sg_ioctl+0xa2/0x180 drivers/scsi/sg.c:1165
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:874 [inline]
 __se_sys_ioctl fs/ioctl.c:860 [inline]
 __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Reported-by: syzkaller <syzkaller@googlegroups.com>
Signed-off-by: George Kennedy <george.kennedy@oracle.com>
---
 drivers/scsi/scsi_debug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 40b473e..222e985 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -3909,7 +3909,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
 		return ret;
 	dnum = 2 * num;
 	arr = kcalloc(lb_size, dnum, GFP_ATOMIC);
-	if (NULL == arr) {
+	if (ZERO_OR_NULL_PTR(arr)) {
 		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
 				INSUFF_RES_ASCQ);
 		return check_condition_result;
@@ -4265,7 +4265,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 		return ret;
 
 	arr = kcalloc(lb_size, vnum, GFP_ATOMIC);
-	if (!arr) {
+	if (ZERO_OR_NULL_PTR(arr)) {
 		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
 				INSUFF_RES_ASCQ);
 		return check_condition_result;
@@ -4334,7 +4334,7 @@ static int resp_report_zones(struct scsi_cmnd *scp,
 			    max_zones);
 
 	arr = kcalloc(RZONES_DESC_HD, alloc_len, GFP_ATOMIC);
-	if (!arr) {
+	if (ZERO_OR_NULL_PTR(arr)) {
 		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
 				INSUFF_RES_ASCQ);
 		return check_condition_result;
-- 
1.8.3.1

Re: [PATCH] scsi: scsi_debug: fix return checks for kcalloc

[Greg KH]

On Wed, Nov 03, 2021 at 02:01:42PM -0500, George Kennedy wrote:
> Change return checks from kcalloc() to now check for NULL and
> ZERO_SIZE_PTR using the ZERO_OR_NULL_PTR macro or the following
> crash can occur if ZERO_SIZE_PTR indicator is returned.

That seems really broken in the api, why is kcalloc() returning
ZERO_SIZE_PTR?

Please fix that, otherwise you need to fix all callers in the kernel
tree.

thanks,

greg k-h

Re: [PATCH] scsi: scsi_debug: fix return checks for kcalloc

[Dan Carpenter]

This patch isn't right.  It has no effect on run time.

On Wed, Nov 03, 2021 at 02:01:42PM -0500, George Kennedy wrote:
> Change return checks from kcalloc() to now check for NULL and
> ZERO_SIZE_PTR using the ZERO_OR_NULL_PTR macro or the following
> crash can occur if ZERO_SIZE_PTR indicator is returned.

ZERO_SIZE_PTR is when you allocate a zero bytes successfully.  It's
quite useful because you can do:

	p = kmalloc(bytes, GFP_KERNEL);

	for (i = 0; i < bytes; i++)
		printk("%d\n", p[i]);

The for loop works.  Zero bytes now like normal thing instead of needing
to be handled as a special case.

The IS_ERR_OR_NULL() check treats ZERO_SIZE_PTR as a valid pointer.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 40b473e..222e985 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -3909,7 +3909,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
>  		return ret;
>  	dnum = 2 * num;
>  	arr = kcalloc(lb_size, dnum, GFP_ATOMIC);
> -	if (NULL == arr) {
> +	if (ZERO_OR_NULL_PTR(arr)) {

kcalloc() only returns NULL on error.  This check is Yoda code but
besides the style issue it's fine.

>  		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>  				INSUFF_RES_ASCQ);
>  		return check_condition_result;
> @@ -4265,7 +4265,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  		return ret;
>  
>  	arr = kcalloc(lb_size, vnum, GFP_ATOMIC);
> -	if (!arr) {

The rest of these are correct.  This patch doesn't fix a bug...

regards,
dan carpenter

Re: [PATCH] scsi: scsi_debug: fix return checks for kcalloc

[George Kennedy]

On 11/4/2021 2:25 AM, Greg KH wrote:
> On Wed, Nov 03, 2021 at 02:01:42PM -0500, George Kennedy wrote:
>> Change return checks from kcalloc() to now check for NULL and
>> ZERO_SIZE_PTR using the ZERO_OR_NULL_PTR macro or the following
>> crash can occur if ZERO_SIZE_PTR indicator is returned.
> That seems really broken in the api, why is kcalloc() returning
> ZERO_SIZE_PTR?
See Dan Carpenter's explanation.

kcalloc() purposely returns ZERO_SIZE_PTR if its size arg is zero.

See commit: 6cb8f91320d3e720351c21741da795fed580b21b

>
> Please fix that, otherwise you need to fix all callers in the kernel
> tree.

Here are the kcalloc() args:
/**
  * kcalloc - allocate memory for an array. The memory is set to zero.
  * @n: number of elements.
  * @size: element size.
  * @flags: the type of memory to allocate (see kmalloc).
  */
static inline void *kcalloc(size_t n, size_t size, gfp_t flags)

Any call to kcalloc() where the size arg (the 2nd arg) can possibly be 
zero needs to check for ZERO_SIZE_PTR being returned along with checking 
for NULL being returned, which the ZERO_OR_NULL_PTR macro does.

In most cases throughout the kernel the calls to kcalloc() are with the 
size arg set to a sizeof some data structure, so ZERO_SIZE_PTR will not 
be returned and a following check for NULL being returned is all that is 
needed.

Thank you,
George

>
> thanks,
>
> greg k-h

Re: [PATCH] scsi: scsi_debug: fix return checks for kcalloc

[Joe Perches]

On Wed, 2021-11-03 at 14:01 -0500, George Kennedy wrote:
> Change return checks from kcalloc() to now check for NULL and
> ZERO_SIZE_PTR using the ZERO_OR_NULL_PTR macro or the following
> crash can occur if ZERO_SIZE_PTR indicator is returned.
> 
> BUG: KASAN: null-ptr-deref in memcpy include/linux/fortify-string.h:191 [inline]
> BUG: KASAN: null-ptr-deref in sg_copy_buffer+0x138/0x240 lib/scatterlist.c:974
> Write of size 4 at addr 0000000000000010 by task syz-executor.1/22789
[]
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
[]
> @@ -3909,7 +3909,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
>  		return ret;
>  	dnum = 2 * num;
>  	arr = kcalloc(lb_size, dnum, GFP_ATOMIC);
> -	if (NULL == arr) {
> +	if (ZERO_OR_NULL_PTR(arr)) {
>  		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>  				INSUFF_RES_ASCQ);
>  		return check_condition_result;

This one isn't necessary as num is already tested for non-0 above
this block.

> @@ -4265,7 +4265,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  		return ret;
>  
>  	arr = kcalloc(lb_size, vnum, GFP_ATOMIC);
> -	if (!arr) {
> +	if (ZERO_OR_NULL_PTR(arr)) {
>  		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>  				INSUFF_RES_ASCQ);
>  		return check_condition_result;

Here it's probably clearer code to test vnum == 0 before the kcalloc
and return check_condition_result;

> @@ -4334,7 +4334,7 @@ static int resp_report_zones(struct scsi_cmnd *scp,
>  			    max_zones);
>  
>  	arr = kcalloc(RZONES_DESC_HD, alloc_len, GFP_ATOMIC);
> -	if (!arr) {
> +	if (ZERO_OR_NULL_PTR(arr)) {
>  		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>  				INSUFF_RES_ASCQ);
>  		return check_condition_result;

And here test alloc_len == 0 before the kcalloc.

Re: [PATCH] scsi: scsi_debug: fix return checks for kcalloc

[George Kennedy]

Thanks Joe,

On 11/4/2021 12:04 PM, Joe Perches wrote:
> On Wed, 2021-11-03 at 14:01 -0500, George Kennedy wrote:
>> Change return checks from kcalloc() to now check for NULL and
>> ZERO_SIZE_PTR using the ZERO_OR_NULL_PTR macro or the following
>> crash can occur if ZERO_SIZE_PTR indicator is returned.
>>
>> BUG: KASAN: null-ptr-deref in memcpy include/linux/fortify-string.h:191 [inline]
>> BUG: KASAN: null-ptr-deref in sg_copy_buffer+0x138/0x240 lib/scatterlist.c:974
>> Write of size 4 at addr 0000000000000010 by task syz-executor.1/22789
> []
>> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> []
>> @@ -3909,7 +3909,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
>>   		return ret;
>>   	dnum = 2 * num;
>>   	arr = kcalloc(lb_size, dnum, GFP_ATOMIC);
>> -	if (NULL == arr) {
>> +	if (ZERO_OR_NULL_PTR(arr)) {
>>   		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>>   				INSUFF_RES_ASCQ);
>>   		return check_condition_result;
> This one isn't necessary as num is already tested for non-0 above
> this block.

The check for "num" preceding the above does this:

         if (0 == num)
                 return 0;       /* degenerate case, not an error */

Shouldn't I use that same size check and "return 0" if size == zero in 
the other cases?

>
>> @@ -4265,7 +4265,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>>   		return ret;
>>   
>>   	arr = kcalloc(lb_size, vnum, GFP_ATOMIC);
>> -	if (!arr) {
>> +	if (ZERO_OR_NULL_PTR(arr)) {
>>   		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>>   				INSUFF_RES_ASCQ);
>>   		return check_condition_result;
> Here it's probably clearer code to test vnum == 0 before the kcalloc
> and return check_condition_result;
>
>> @@ -4334,7 +4334,7 @@ static int resp_report_zones(struct scsi_cmnd *scp,
>>   			    max_zones);
>>   
>>   	arr = kcalloc(RZONES_DESC_HD, alloc_len, GFP_ATOMIC);
>> -	if (!arr) {
>> +	if (ZERO_OR_NULL_PTR(arr)) {
>>   		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>>   				INSUFF_RES_ASCQ);
>>   		return check_condition_result;
> And here test alloc_len == 0 before the kcalloc.

Using your suggested fix (with return 0 instead of return 
check_condition_result) the patch would look like this:

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 40b473e..93913d2 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -4258,6 +4258,8 @@ static int resp_verify(struct scsi_cmnd *scp, 
struct sdebug_dev_info *devip)
                 mk_sense_invalid_opcode(scp);
                 return check_condition_result;
         }
+       if (0 == vnum)
+               return 0;       /* degenerate case, not an error */
         a_num = is_bytchk3 ? 1 : vnum;
         /* Treat following check like one for read (i.e. no write) 
access */
         ret = check_device_access_params(scp, lba, a_num, false);
@@ -4321,6 +4323,8 @@ static int resp_report_zones(struct scsi_cmnd *scp,
         }
         zs_lba = get_unaligned_be64(cmd + 2);
         alloc_len = get_unaligned_be32(cmd + 10);
+       if (0 == alloc_len)
+               return 0;       /* degenerate case, not an error */
         rep_opts = cmd[14] & 0x3f;
         partial = cmd[14] & 0x80;


Does the above look ok?

George

>
>

Re: [PATCH] scsi: scsi_debug: fix return checks for kcalloc

[Douglas Gilbert]

On 2021-11-03 3:01 p.m., George Kennedy wrote:
> Change return checks from kcalloc() to now check for NULL and
> ZERO_SIZE_PTR using the ZERO_OR_NULL_PTR macro or the following
> crash can occur if ZERO_SIZE_PTR indicator is returned.
> 
> BUG: KASAN: null-ptr-deref in memcpy include/linux/fortify-string.h:191 [inline]
> BUG: KASAN: null-ptr-deref in sg_copy_buffer+0x138/0x240 lib/scatterlist.c:974
> Write of size 4 at addr 0000000000000010 by task syz-executor.1/22789
> 
> CPU: 1 PID: 22789 Comm: syz-executor.1 Not tainted 5.15.0-syzk #1
> Hardware name: Red Hat KVM, BIOS 1.13.0-2
> Call Trace:
>   __dump_stack lib/dump_stack.c:88 [inline]
>   dump_stack_lvl+0x89/0xb5 lib/dump_stack.c:106
>   __kasan_report mm/kasan/report.c:446 [inline]
>   kasan_report.cold.14+0x112/0x117 mm/kasan/report.c:459
>   check_region_inline mm/kasan/generic.c:183 [inline]
>   kasan_check_range+0x1a3/0x210 mm/kasan/generic.c:189
>   memcpy+0x3b/0x60 mm/kasan/shadow.c:66
>   memcpy include/linux/fortify-string.h:191 [inline]
>   sg_copy_buffer+0x138/0x240 lib/scatterlist.c:974
>   do_dout_fetch drivers/scsi/scsi_debug.c:2954 [inline]
>   do_dout_fetch drivers/scsi/scsi_debug.c:2946 [inline]
>   resp_verify+0x49e/0x930 drivers/scsi/scsi_debug.c:4276
>   schedule_resp+0x4d8/0x1a70 drivers/scsi/scsi_debug.c:5478
>   scsi_debug_queuecommand+0x8c9/0x1ec0 drivers/scsi/scsi_debug.c:7533
>   scsi_dispatch_cmd drivers/scsi/scsi_lib.c:1520 [inline]
>   scsi_queue_rq+0x16b0/0x2d40 drivers/scsi/scsi_lib.c:1699
>   blk_mq_dispatch_rq_list+0xb9b/0x2700 block/blk-mq.c:1639
>   __blk_mq_sched_dispatch_requests+0x28f/0x590 block/blk-mq-sched.c:325
>   blk_mq_sched_dispatch_requests+0x105/0x190 block/blk-mq-sched.c:358
>   __blk_mq_run_hw_queue+0xe5/0x150 block/blk-mq.c:1761
>   __blk_mq_delay_run_hw_queue+0x4f8/0x5c0 block/blk-mq.c:1838
>   blk_mq_run_hw_queue+0x18d/0x350 block/blk-mq.c:1891
>   blk_mq_sched_insert_request+0x3db/0x4e0 block/blk-mq-sched.c:474
>   blk_execute_rq_nowait+0x16b/0x1c0 block/blk-exec.c:62
>   blk_execute_rq+0xdb/0x360 block/blk-exec.c:102
>   sg_scsi_ioctl drivers/scsi/scsi_ioctl.c:621 [inline]
>   scsi_ioctl+0x8bb/0x15c0 drivers/scsi/scsi_ioctl.c:930
>   sg_ioctl_common+0x172d/0x2710 drivers/scsi/sg.c:1112
>   sg_ioctl+0xa2/0x180 drivers/scsi/sg.c:1165
>   vfs_ioctl fs/ioctl.c:51 [inline]
>   __do_sys_ioctl fs/ioctl.c:874 [inline]
>   __se_sys_ioctl fs/ioctl.c:860 [inline]
>   __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860
>   do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>   do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80
>   entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Reported-by: syzkaller <syzkaller@googlegroups.com>
> Signed-off-by: George Kennedy <george.kennedy@oracle.com>
> ---
>   drivers/scsi/scsi_debug.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 40b473e..222e985 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -3909,7 +3909,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
>   		return ret;
>   	dnum = 2 * num;
>   	arr = kcalloc(lb_size, dnum, GFP_ATOMIC);
> -	if (NULL == arr) {
> +	if (ZERO_OR_NULL_PTR(arr)) {
>   		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>   				INSUFF_RES_ASCQ);
>   		return check_condition_result;
> @@ -4265,7 +4265,7 @@ static int resp_verify(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>   		return ret;

This is related to a field in a SCSI command (e.g. a READ) that dictates
the size of the data-in transfer. In all cases that I can think of the
standards say it is _not_ an error if that field is zero. So it is
incorrect to report an ILLEGAL_REQUEST in that case, those commands
should do nothing (apart from check the other fields (e.g. for an out of
range starting LBA)) and return GOOD status.

So this patch is incorrect.

Doug Gilbert

>   
>   	arr = kcalloc(lb_size, vnum, GFP_ATOMIC);
> -	if (!arr) {
> +	if (ZERO_OR_NULL_PTR(arr)) {
>   		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>   				INSUFF_RES_ASCQ);
>   		return check_condition_result;
> @@ -4334,7 +4334,7 @@ static int resp_report_zones(struct scsi_cmnd *scp,
>   			    max_zones);
>   
>   	arr = kcalloc(RZONES_DESC_HD, alloc_len, GFP_ATOMIC);
> -	if (!arr) {
> +	if (ZERO_OR_NULL_PTR(arr)) {
>   		mk_sense_buffer(scp, ILLEGAL_REQUEST, INSUFF_RES_ASC,
>   				INSUFF_RES_ASCQ);
>   		return check_condition_result;
>