LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] scsi: sg: fix a missing-check bug
@ 2018-05-06  3:21 Wenwen Wang
  2018-05-07  5:13 ` Douglas Gilbert
  0 siblings, 1 reply; 3+ messages in thread
From: Wenwen Wang @ 2018-05-06  3:21 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, Doug Gilbert, James E.J. Bottomley,
	Martin K. Petersen, open list:SCSI SG DRIVER, open list

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@umn.edu>
---
 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,
-- 
2.7.4

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] scsi: sg: fix a missing-check bug
  2018-05-06  3:21 [PATCH] scsi: sg: fix a missing-check bug Wenwen Wang
@ 2018-05-07  5:13 ` Douglas Gilbert
  2018-05-18 21:48   ` Wenwen Wang
  0 siblings, 1 reply; 3+ messages in thread
From: Douglas Gilbert @ 2018-05-07  5:13 UTC (permalink / raw)
  To: Wenwen Wang
  Cc: Kangjie Lu, James E.J. Bottomley, Martin K. Petersen,
	open list:SCSI SG DRIVER, open list

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@umn.edu>
> ---
>   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?

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

Doug Gilbert

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] scsi: sg: fix a missing-check bug
  2018-05-07  5:13 ` Douglas Gilbert
@ 2018-05-18 21:48   ` Wenwen Wang
  0 siblings, 0 replies; 3+ messages in thread
From: Wenwen Wang @ 2018-05-18 21:48 UTC (permalink / raw)
  To: dgilbert
  Cc: Kangjie Lu, James E.J. Bottomley, Martin K. Petersen,
	open list:SCSI SG DRIVER, open list, Wenwen Wang

On Mon, May 7, 2018 at 12:13 AM, Douglas Gilbert <dgilbert@interlog.com> 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@umn.edu>
>> ---
>>   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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-05-18 21:49 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-06  3:21 [PATCH] scsi: sg: fix a missing-check bug Wenwen Wang
2018-05-07  5:13 ` Douglas Gilbert
2018-05-18 21:48   ` Wenwen Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).