Re: [PATCH] scsi: sg: fix a missing-check bug

[Wenwen Wang]

On Mon, May 7, 2018 at 12:13 AM, Douglas Gilbert <dgilbert@xxxxxxxxxxxx> wrote:
> On 2018-05-05 11:21 PM, Wenwen Wang wrote:
>>
>> In sg_write(), the opcode of the command is firstly copied from the
>> userspace pointer 'buf' and saved to the kernel variable 'opcode', using
>> the __get_user() function. The size of the command, i.e., 'cmd_size' is
>> then calculated based on the 'opcode'. After that, the whole command,
>> including the opcode, is copied again from 'buf' using the
>> __copy_from_user() function and saved to 'cmnd'. Finally, the function
>>   sg_common_write() is invoked to process 'cmnd'. Given that the 'buf'
>> pointer resides in userspace, a malicious userspace process can race to
>> change the opcode of the command between the two copies. That means, the
>> opcode indicated by the variable 'opcode' could be different from the
>> opcode in 'cmnd'. This can cause inconsistent data in 'cmnd' and
>> potential logical errors in the function sg_common_write(), as it needs to
>> work on 'cmnd'.
>>
>> This patch reuses the opcode obtained in the first copy and only copies
>> the
>> remaining part of the command from userspace.
>>
>> Signed-off-by: Wenwen Wang <wang6495@xxxxxxx>
>> ---
>>   drivers/scsi/sg.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index c198b963..0ad8106 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -657,7 +657,8 @@ sg_write(struct file *filp, const char __user *buf,
>> size_t count, loff_t * ppos)
>>         hp->flags = input_size; /* structure abuse ... */
>>         hp->pack_id = old_hdr.pack_id;
>>         hp->usr_ptr = NULL;
>> -       if (__copy_from_user(cmnd, buf, cmd_size))
>> +       cmnd[0] = opcode;
>> +       if (__copy_from_user(cmnd + 1, buf + 1, cmd_size - 1))
>>                 return -EFAULT;
>>         /*
>>          * SG_DXFER_TO_FROM_DEV is functionally equivalent to
>> SG_DXFER_FROM_DEV,
>>
>
> That is in the deprecated "v2" part of the sg driver (for around 15 years).
> There are lots more interesting races with that interface than that one
> described above. My guess is that all system calls would be susceptible
> to playing around with a buffer being passed to or from the OS by a thread
> other than the one doing the system call, during that call. Surely no Unix
> like OS gives any security guarantees to a thread being attacked by a
> malevolent thread in the same process!
>
> My question is did this actually cause to program to fail; or is it
> something
> that a sanity checker flagged?

This is detected by a static analysis tool. But, based on our manual
investigation, it can cause program failure. So it is better to fix
it.

>
> Also wouldn't it be better just to return an error such as EINVAL if
> opcode != command[0]  ?

I can revise this patch to return EINVAL if the opcode is not as
expected, and resubmit it.

Thanks!

Wenwen