[PATCH] scsi: core: initialize cmd->cmnd before it is used

[PATCH] scsi: core: initialize cmd->cmnd before it is used

[Tadeusz Struk]

The scsi_setup_scsi_cmnd() function initializes the cmd->cmnd command
field, but in case the cmd_len is zero the field is used to detect
the command size before it is initialized. This triggers null-ptr-deref
as in the trace below. Fix this by setting cmd->cmnd earlier.

[   21.105583][ T1822] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
[   21.106749][ T1822] CPU: 0 PID: 1822 Comm: repro Not tainted 5.15.0 #1
[   21.107678][ T1822] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-4.fc34 04/01/2014
[   21.109004][ T1822] RIP: 0010:scsi_queue_rq+0xf09/0x2180
[   21.112499][ T1822] RSP: 0018:ffffc90000d0f098 EFLAGS: 00010246
[   21.113347][ T1822] RAX: dffffc0000000000 RBX: ffff888107b0d408 RCX: 0000000000000000
[   21.114448][ T1822] RDX: ffff888107168000 RSI: 0000000000000000 RDI: 0000000000000000
[   21.115553][ T1822] RBP: ffffc90000d0f150 R08: ffffffff82a96d37 R09: ffff888107b0d410
[   21.116683][ T1822] R10: ffffed1020f61a85 R11: 0000000000000000 R12: 1ffff11020f61a7f
[   21.117793][ T1822] R13: 0000000000000000 R14: 0000000000000000 R15: ffff888107b0d3fc
[   21.118894][ T1822] FS:  00007f5bfac9f640(0000) GS:ffff88811b200000(0000) knlGS:0000000000000000
[   21.120132][ T1822] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   21.121050][ T1822] CR2: 0000000020001000 CR3: 0000000109acb000 CR4: 0000000000150eb0
[   21.123125][ T1822] Call Trace:
[   21.124020][ T1822]  blk_mq_dispatch_rq_list+0x7c7/0x12d0
[   21.125486][ T1822]  ? __kasan_check_write+0x14/0x20
[   21.126794][ T1822]  ? do_raw_spin_lock+0x9a/0x230
[   21.128023][ T1822]  ? blk_mq_get_driver_tag+0x920/0x920
[   21.129345][ T1822]  __blk_mq_sched_dispatch_requests+0x244/0x380
[   21.130826][ T1822]  ? blk_mq_sched_dispatch_requests+0x160/0x160
[   21.132241][ T1822]  ? _find_next_bit+0x1ec/0x210
[   21.133376][ T1822]  blk_mq_sched_dispatch_requests+0xf0/0x160
[   21.134724][ T1822]  __blk_mq_run_hw_queue+0xe8/0x160
[   21.135878][ T1822]  ? __list_splice+0x100/0x100
[   21.136902][ T1822]  __blk_mq_delay_run_hw_queue+0x252/0x5d0
[   21.138115][ T1822]  blk_mq_run_hw_queue+0x1dd/0x3b0
[   21.139184][ T1822]  ? blk_mq_dispatch_rq_list+0x12d0/0x12d0
[   21.140322][ T1822]  ? _raw_spin_unlock+0x13/0x30
[   21.141273][ T1822]  ? blk_mq_request_bypass_insert+0xcf/0xe0
[   21.142414][ T1822]  blk_mq_sched_insert_request+0x1ff/0x3e0
[   21.143531][ T1822]  ? timekeeping_get_ns+0xb1/0xc0
[   21.144462][ T1822]  ? blk_mq_sched_try_insert_merge+0x240/0x240
[   21.145592][ T1822]  ? update_io_ticks+0x17c/0x190
[   21.146472][ T1822]  ? blk_account_io_start+0x21c/0x260
[   21.147421][ T1822]  blk_execute_rq_nowait+0x173/0x1e0
[   21.148356][ T1822]  ? blk_execute_rq+0x540/0x540
[   21.149247][ T1822]  ? asan.module_ctor+0x10/0x10
[   21.150066][ T1822]  blk_execute_rq+0x15c/0x540
[   21.150869][ T1822]  ? cap_capable+0x2ca/0x330
[   21.151653][ T1822]  ? blk_execute_rq_nowait+0x1e0/0x1e0
[   21.152539][ T1822]  ? ns_capable_common+0x8f/0xf0
[   21.153348][ T1822]  ? capable+0x1c/0x20
[   21.153984][ T1822]  sg_io+0x97c/0x1370
[   21.154614][ T1822]  ? scsi_ioctl_block_when_processing_errors+0x1e0/0x1e0
[   21.155712][ T1822]  ? in_compat_syscall+0xd0/0xd0
[   21.156488][ T1822]  ? futex_wait+0x4fb/0x640
[   21.157175][ T1822]  scsi_ioctl+0xe16/0x28e0
[   21.157836][ T1822]  ? __kasan_check_read+0x11/0x20
[   21.158596][ T1822]  ? get_sg_io_hdr+0x10a0/0x10a0
[   21.159347][ T1822]  ? __fsnotify_parent+0x4ee/0x710
[   21.160111][ T1822]  ? do_futex+0x3f2/0x1030
[   21.160764][ T1822]  ? futex_exit_release+0x70/0x70
[   21.161494][ T1822]  ? do_vfs_ioctl+0xafa/0x1af0
[   21.162168][ T1822]  ? scsi_host_in_recovery+0xb9/0x160
[   21.162921][ T1822]  ? vfs_write+0x397/0x580
[   21.163547][ T1822]  ? scsi_ioctl_block_when_processing_errors+0xae/0x1e0
[   21.164521][ T1822]  sd_ioctl+0x134/0x170
[   21.165087][ T1822]  blkdev_ioctl+0x362/0x6e0
[   21.165724][ T1822]  ? blkdev_compat_ptr_ioctl+0xf0/0xf0
[   21.166470][ T1822]  ? fput_many+0x5e/0x1d0
[   21.167059][ T1822]  ? __restore_fpregs_from_fpstate+0xb5/0x160
[   21.167873][ T1822]  block_ioctl+0xb0/0xf0
[   21.168437][ T1822]  vfs_ioctl+0xa7/0xf0
[   21.168970][ T1822]  __x64_sys_ioctl+0x10d/0x150
[   21.169602][ T1822]  do_syscall_64+0x3d/0xb0
[   21.170176][ T1822]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   21.180158][ T1822] ---[ end trace 8b086e334adef6d2 ]---
[   21.191043][ T1822] Kernel panic - not syncing: Fatal exception
[   21.191728][ T1822] Kernel Offset: disabled

Cc: Christoph Hellwig <hch@lst.de>
Cc: James E.J. Bottomley <jejb@linux.ibm.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: <linux-scsi@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Cc: <stable@vger.kernel.org> # 5.15, 5.14, 5.10
Fixes: 2ceda20f0a99a74a82b78870f3b3e5fa93087a7f
Reported-by: syzbot+5516b30f5401d4dcbcae@syzkaller.appspotmail.com
Signed-off-by: Tadeusz Struk <tadeusz.struk@linaro.org>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 572673873ddf..cd4b57747448 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1173,10 +1173,10 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
 		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
 	}
 
+	cmd->cmnd = scsi_req(req)->cmd;
 	cmd->cmd_len = scsi_req(req)->cmd_len;
 	if (cmd->cmd_len == 0)
 		cmd->cmd_len = scsi_command_size(cmd->cmnd);
-	cmd->cmnd = scsi_req(req)->cmd;
 	cmd->transfersize = blk_rq_bytes(req);
 	cmd->allowed = scsi_req(req)->retries;
 	return BLK_STS_OK;
-- 
2.31.1

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Bart Van Assche]

On 11/1/21 12:24 PM, Tadeusz Struk wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 572673873ddf..cd4b57747448 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1173,10 +1173,10 @@ static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
>   		memset(&cmd->sdb, 0, sizeof(cmd->sdb));
>   	}
>   
> +	cmd->cmnd = scsi_req(req)->cmd;
>   	cmd->cmd_len = scsi_req(req)->cmd_len;
>   	if (cmd->cmd_len == 0)
>   		cmd->cmd_len = scsi_command_size(cmd->cmnd);
> -	cmd->cmnd = scsi_req(req)->cmd;
>   	cmd->transfersize = blk_rq_bytes(req);
>   	cmd->allowed = scsi_req(req)->retries;
>   	return BLK_STS_OK;

This patch is a duplicate and has been posted before.

Please take a look at https://lore.kernel.org/linux-scsi/20210904064534.1919476-1-qiulaibin@huawei.com/.
 From the replies to that email:
"> Thinking further about this: is there any code left that depends on
 > scsi_setup_scsi_cmnd() setting cmd->cmd_len? Can the cmd->cmd_len
 > assignment be removed from scsi_setup_scsi_cmnd()?

cmd_len should never be 0 now, so I think we can remove it."

Thanks,

Bart.

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Tadeusz Struk]

On 11/1/21 13:06, Bart Van Assche wrote:
> This patch is a duplicate and has been posted before.
> 
> Please take a look at 
> https://lore.kernel.org/linux-scsi/20210904064534.1919476-1-qiulaibin@huawei.com/.
>  From the replies to that email:
> "> Thinking further about this: is there any code left that depends on
>  > scsi_setup_scsi_cmnd() setting cmd->cmd_len? Can the cmd->cmd_len
>  > assignment be removed from scsi_setup_scsi_cmnd()?
> 
> cmd_len should never be 0 now, so I think we can remove it."

Thanks for quick response, but I'm not sure if statement
"cmd_len should never be 0 now" is correct, because the cmd_len is
in fact equal to 0 here and this BUG can be triggered on mainline, 5.14,
and 5.10 stable kernels.

-- 
Thanks,
Tadeusz

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Bart Van Assche]

On 11/1/21 1:13 PM, Tadeusz Struk wrote:
> On 11/1/21 13:06, Bart Van Assche wrote:
>> This patch is a duplicate and has been posted before.
>>
>> Please take a look at https://lore.kernel.org/linux-scsi/20210904064534.1919476-1-qiulaibin@huawei.com/.
>>  From the replies to that email:
>> "> Thinking further about this: is there any code left that depends on
>>  > scsi_setup_scsi_cmnd() setting cmd->cmd_len? Can the cmd->cmd_len
>>  > assignment be removed from scsi_setup_scsi_cmnd()?
>>
>> cmd_len should never be 0 now, so I think we can remove it."
> 
> Thanks for quick response, but I'm not sure if statement
> "cmd_len should never be 0 now" is correct, because the cmd_len is
> in fact equal to 0 here and this BUG can be triggered on mainline, 5.14,
> and 5.10 stable kernels.

(+Doug Gilbert)

One of the functions in the call stack in the first message of this email
thread is sg_io(). I am not aware of any documentation that specifies whether
it is valid to set cmd_len in the sg_io header to zero. My opinion is that
the SG_IO implementation should either reject cmd_len == 0 or set cmd_len
to a valid value if it is zero.

Bart.

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Douglas Gilbert]

On 2021-11-01 4:20 p.m., Bart Van Assche wrote:
> On 11/1/21 1:13 PM, Tadeusz Struk wrote:
>> On 11/1/21 13:06, Bart Van Assche wrote:
>>> This patch is a duplicate and has been posted before.
>>>
>>> Please take a look at 
>>> https://lore.kernel.org/linux-scsi/20210904064534.1919476-1-qiulaibin@huawei.com/. 
>>>
>>>  From the replies to that email:
>>> "> Thinking further about this: is there any code left that depends on
>>>  > scsi_setup_scsi_cmnd() setting cmd->cmd_len? Can the cmd->cmd_len
>>>  > assignment be removed from scsi_setup_scsi_cmnd()?
>>>
>>> cmd_len should never be 0 now, so I think we can remove it."
>>
>> Thanks for quick response, but I'm not sure if statement
>> "cmd_len should never be 0 now" is correct, because the cmd_len is
>> in fact equal to 0 here and this BUG can be triggered on mainline, 5.14,
>> and 5.10 stable kernels.
> 
> (+Doug Gilbert)
> 
> One of the functions in the call stack in the first message of this email
> thread is sg_io(). I am not aware of any documentation that specifies whether
> it is valid to set cmd_len in the sg_io header to zero. My opinion is that
> the SG_IO implementation should either reject cmd_len == 0 or set cmd_len
> to a valid value if it is zero.

For the sg driver in production, the v3 interface users (including
ioctl(<sg_fd>, SG_IO,) ) have this check:

        if ((!hp->cmdp) || (hp->cmd_len < 6) || (hp->cmd_len > sizeof (cmnd))) {
                 sg_remove_request(sfp, srp);
                 return -EMSGSIZE;
         }

For the v1 and v2 interface users there was no cmd_len. It was deduced via
COMMAND_SIZE(opcode) or by calling ioctl(SG_NEXT_CMD_LEN) prior to the write()
to issue the SCSI command.

Looking at the block layer/ SCSI mid level implementation of ioctl(SG_IO) I
can see no lower bound check on cmd_len (which is 'unsigned char' so at least
it can't go negative).

Doug Gilbert

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Bart Van Assche]

On 11/1/21 18:56, Douglas Gilbert wrote:
> On 2021-11-01 4:20 p.m., Bart Van Assche wrote:
>> One of the functions in the call stack in the first message of this email
>> thread is sg_io(). I am not aware of any documentation that specifies 
>> whether
>> it is valid to set cmd_len in the sg_io header to zero. My opinion is 
>> that
>> the SG_IO implementation should either reject cmd_len == 0 or set cmd_len
>> to a valid value if it is zero.
> 
> For the sg driver in production, the v3 interface users (including
> ioctl(<sg_fd>, SG_IO,) ) have this check:
> 
>         if ((!hp->cmdp) || (hp->cmd_len < 6) || (hp->cmd_len > sizeof 
> (cmnd))) {
>                  sg_remove_request(sfp, srp);
>                  return -EMSGSIZE;
>          }

Hi Doug,

Thanks for having taken a look. I found the above check in 
sg_new_write(). To me that function seems to come from a code path that 
is unrelated to sg_io(), the function shown in the call stack in the 
email at the start of this thread. Maybe I overlooked something but I 
haven't found a minimum size check for hdr->cmd_len in sg_io() before 
the blk_execute_rq() call. Should such a check perhaps be added?

Thanks,

Bart.

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Douglas Gilbert]

On 2021-11-01 11:06 p.m., Bart Van Assche wrote:
> On 11/1/21 18:56, Douglas Gilbert wrote:
>> On 2021-11-01 4:20 p.m., Bart Van Assche wrote:
>>> One of the functions in the call stack in the first message of this email
>>> thread is sg_io(). I am not aware of any documentation that specifies whether
>>> it is valid to set cmd_len in the sg_io header to zero. My opinion is that
>>> the SG_IO implementation should either reject cmd_len == 0 or set cmd_len
>>> to a valid value if it is zero.
>>
>> For the sg driver in production, the v3 interface users (including
>> ioctl(<sg_fd>, SG_IO,) ) have this check:
>>
>>         if ((!hp->cmdp) || (hp->cmd_len < 6) || (hp->cmd_len > sizeof (cmnd))) {
>>                  sg_remove_request(sfp, srp);
>>                  return -EMSGSIZE;
>>          }
> 
> Hi Doug,
> 
> Thanks for having taken a look. I found the above check in sg_new_write(). To me 
> that function seems to come from a code path that is unrelated to sg_io(), the 
> function shown in the call stack in the email at the start of this thread. Maybe 
> I overlooked something but I haven't found a minimum size check for hdr->cmd_len 
> in sg_io() before the blk_execute_rq() call. Should such a check perhaps be added?

I guess it came from ioctl(<non_sg_fd>, SG_IO, ) and I found no lower bound
check when I looked in lk 5.15.0 . No-one has complained to me about the
    hp->cmd_len < 6

check in the sg driver ***. So I think such a check may be useful in the 
scsi_fill_sghdr_rq() function in drivers/scsi/scsi_ioctl.c . And a return
of -EMSGSIZE seems to be tailor made for this situation.

Doug Gilbert


*** It is possible a vendor specific command could be between 1 and 5 bytes
long, but that would probably be an unwise choice.

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Tadeusz Struk]

On 11/2/21 14:33, Douglas Gilbert wrote:
>> Thanks for having taken a look. I found the above check in sg_new_write(). To 
>> me that function seems to come from a code path that is unrelated to sg_io(), 
>> the function shown in the call stack in the email at the start of this thread. 
>> Maybe I overlooked something but I haven't found a minimum size check for 
>> hdr->cmd_len in sg_io() before the blk_execute_rq() call. Should such a check 
>> perhaps be added?
> 
> I guess it came from ioctl(<non_sg_fd>, SG_IO, ) and I found no lower bound
> check when I looked in lk 5.15.0 . No-one has complained to me about the
>     hp->cmd_len < 6
> 
> check in the sg driver ***. So I think such a check may be useful in the 
> scsi_fill_sghdr_rq() function in drivers/scsi/scsi_ioctl.c . And a return
> of -EMSGSIZE seems to be tailor made for this situation.
> 
> Doug Gilbert
> 
> 
> *** It is possible a vendor specific command could be between 1 and 5 bytes
> long, but that would probably be an unwise choice.

Bart,
Do you want me to send a patch with the check in scsi_fill_sghdr_rq()?
I want to close the mentioned syzbot issue in 5.10. I can also do the
back-porting if anything will be required.

-- 
Thanks,
Tadeusz

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Bart Van Assche]

On 11/2/21 3:22 PM, Tadeusz Struk wrote:
> Do you want me to send a patch with the check in scsi_fill_sghdr_rq()?
> I want to close the mentioned syzbot issue in 5.10. I can also do the
> back-porting if anything will be required.

Hi Tadeusz,

I think we need two patches: one for the SG_IO code that rejects SG_IO
requests if the CDB length is not valid and a second patch that removes
the code from scsi_lib.c for assigning the CDB length. Please let me
know if you would not have the time to work on this.

Bart.

Re: [PATCH] scsi: core: initialize cmd->cmnd before it is used

[Tadeusz Struk]

On 11/2/21 15:27, Bart Van Assche wrote:
> On 11/2/21 3:22 PM, Tadeusz Struk wrote:
>> Do you want me to send a patch with the check in scsi_fill_sghdr_rq()?
>> I want to close the mentioned syzbot issue in 5.10. I can also do the
>> back-porting if anything will be required.
> 
> Hi Tadeusz,
> 
> I think we need two patches: one for the SG_IO code that rejects SG_IO
> requests if the CDB length is not valid and a second patch that removes
> the code from scsi_lib.c for assigning the CDB length. Please let me
> know if you would not have the time to work on this.

Yes, I agree. I will send the patches today.

-- 
Thanks,
Tadeusz