LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c
@ 2019-05-23  2:38 Gen Zhang
  2019-06-05  6:00 ` Jiri Slaby
  0 siblings, 1 reply; 4+ messages in thread
From: Gen Zhang @ 2019-05-23  2:38 UTC (permalink / raw)
  To: martin.petersen; +Cc: linux-scsi, linux-kernel

In sg_write(), the opcode of the command is fetched the first time from 
the userspace by __get_user(). Then the whole command, the opcode 
included, is fetched again from userspace by __copy_from_user(). 
However, a malicious user can change the opcode between the two fetches.
This can cause inconsistent data and potential errors as cmnd is used in
the following codes.

Thus we should check opcode between the two fetches to prevent this.

Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
---
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index d3f1531..a2971b8 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -694,6 +694,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 (opcode != cmnd[0])
+		return -EINVAL;
 	if (__copy_from_user(cmnd, buf, cmd_size))
 		return -EFAULT;
 	/*
---

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

* Re: [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c
  2019-05-23  2:38 [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c Gen Zhang
@ 2019-06-05  6:00 ` Jiri Slaby
  2019-06-05 17:07   ` Douglas Gilbert
  0 siblings, 1 reply; 4+ messages in thread
From: Jiri Slaby @ 2019-06-05  6:00 UTC (permalink / raw)
  To: Gen Zhang, martin.petersen; +Cc: linux-scsi, linux-kernel

On 23. 05. 19, 4:38, Gen Zhang wrote:
> In sg_write(), the opcode of the command is fetched the first time from 
> the userspace by __get_user(). Then the whole command, the opcode 
> included, is fetched again from userspace by __copy_from_user(). 
> However, a malicious user can change the opcode between the two fetches.
> This can cause inconsistent data and potential errors as cmnd is used in
> the following codes.
> 
> Thus we should check opcode between the two fetches to prevent this.
> 
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> ---
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index d3f1531..a2971b8 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -694,6 +694,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 (opcode != cmnd[0])
> +		return -EINVAL;

Isn't it too early to check cmnd which is copied only here:

>  	if (__copy_from_user(cmnd, buf, cmd_size))
>  		return -EFAULT;
>  	/*
> ---
> 

thanks,
-- 
js
suse labs

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

* Re: [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c
  2019-06-05  6:00 ` Jiri Slaby
@ 2019-06-05 17:07   ` Douglas Gilbert
  2019-06-06  8:00     ` Gen Zhang
  0 siblings, 1 reply; 4+ messages in thread
From: Douglas Gilbert @ 2019-06-05 17:07 UTC (permalink / raw)
  To: Jiri Slaby, Gen Zhang, martin.petersen; +Cc: linux-scsi, linux-kernel

On 2019-06-05 2:00 a.m., Jiri Slaby wrote:
> On 23. 05. 19, 4:38, Gen Zhang wrote:
>> In sg_write(), the opcode of the command is fetched the first time from
>> the userspace by __get_user(). Then the whole command, the opcode
>> included, is fetched again from userspace by __copy_from_user().
>> However, a malicious user can change the opcode between the two fetches.
>> This can cause inconsistent data and potential errors as cmnd is used in
>> the following codes.
>>
>> Thus we should check opcode between the two fetches to prevent this.
>>
>> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
>> ---
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index d3f1531..a2971b8 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -694,6 +694,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 (opcode != cmnd[0])
>> +		return -EINVAL;
> 
> Isn't it too early to check cmnd which is copied only here:
> 
>>   	if (__copy_from_user(cmnd, buf, cmd_size))
>>   		return -EFAULT;
>>   	/*
>> ---
>>

Hi,
Yes, it is too early. It needs to be after that __copy_from_user(cmnd,
buf, cmd_size) call.

To put this in context, this is a very old interface; dating from 1992
and deprecated for almost 20 years. The fact that the first byte of
the SCSI cdb needs to be read first to work out that size of the
following SCSI command and optionally the offset of a data-out
buffer that may follow the command; is one reason why that interface
was replaced. Also the implementation did not handle SCSI variable
length cdb_s.

Then there is the question of whether this double-fetch is exploitable?
I cannot think of an example, but there might be (e.g. turning a READ
command into a WRITE). But the "double-fetch" issue may be more wide
spread. The replacement interface passes the command and data-in/-out as
pointers while their corresponding lengths are placed in the newer
interface structure. This assumes that the cdb and data-out won't
change in the user space between when the write(2) is called and
before or while the driver, using those pointers, reads the data.
All drivers that use pointers to pass data have this "feature".

Also I'm looking at this particular double-fetch from the point of view
of the driver rewrite I have done and is currently in the early stages
of review [linux-scsi list: "[PATCH 00/19] sg: v4 interface, rq sharing
+ multiple rqs"] and this problem is more difficult to fix since the
full cdb read is delayed to a common point further along the submit
processing path. To detect a change in cbd[0] my current code would
need to be altered to carry cdb[0] through to that common point. So
is it worth it for such an old, deprecated and replaced interface??
What cdb/user_permissions checking that is done, is done _after_
the full cdb is read. So trying to get around a user exclusion of
say WRITE(10) by first using the first byte of READ(10), won't succeed.

Doug Gilbert


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

* Re: [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c
  2019-06-05 17:07   ` Douglas Gilbert
@ 2019-06-06  8:00     ` Gen Zhang
  0 siblings, 0 replies; 4+ messages in thread
From: Gen Zhang @ 2019-06-06  8:00 UTC (permalink / raw)
  To: Douglas Gilbert; +Cc: Jiri Slaby, martin.petersen, linux-scsi, linux-kernel

On Wed, Jun 05, 2019 at 01:07:25PM -0400, Douglas Gilbert wrote:
> On 2019-06-05 2:00 a.m., Jiri Slaby wrote:
> >On 23. 05. 19, 4:38, Gen Zhang wrote:
> >>In sg_write(), the opcode of the command is fetched the first time from
> >>the userspace by __get_user(). Then the whole command, the opcode
> >>included, is fetched again from userspace by __copy_from_user().
> >>However, a malicious user can change the opcode between the two fetches.
> >>This can cause inconsistent data and potential errors as cmnd is used in
> >>the following codes.
> >>
> >>Thus we should check opcode between the two fetches to prevent this.
> >>
> >>Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> >>---
> >>diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> >>index d3f1531..a2971b8 100644
> >>--- a/drivers/scsi/sg.c
> >>+++ b/drivers/scsi/sg.c
> >>@@ -694,6 +694,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 (opcode != cmnd[0])
> >>+		return -EINVAL;
> >
> >Isn't it too early to check cmnd which is copied only here:
> >
> >>  	if (__copy_from_user(cmnd, buf, cmd_size))
> >>  		return -EFAULT;
> >>  	/*
> >>---
> >>
> 
> Hi,
> Yes, it is too early. It needs to be after that __copy_from_user(cmnd,
> buf, cmd_size) call.
Yes, it is.
> 
> To put this in context, this is a very old interface; dating from 1992
> and deprecated for almost 20 years. The fact that the first byte of
> the SCSI cdb needs to be read first to work out that size of the
> following SCSI command and optionally the offset of a data-out
> buffer that may follow the command; is one reason why that interface
> was replaced. Also the implementation did not handle SCSI variable
> length cdb_s.
> 
> Then there is the question of whether this double-fetch is exploitable?
> I cannot think of an example, but there might be (e.g. turning a READ
> command into a WRITE). But the "double-fetch" issue may be more wide
> spread. The replacement interface passes the command and data-in/-out as
> pointers while their corresponding lengths are placed in the newer
> interface structure. This assumes that the cdb and data-out won't
> change in the user space between when the write(2) is called and
> before or while the driver, using those pointers, reads the data.
> All drivers that use pointers to pass data have this "feature".
> 
> Also I'm looking at this particular double-fetch from the point of view
> of the driver rewrite I have done and is currently in the early stages
> of review [linux-scsi list: "[PATCH 00/19] sg: v4 interface, rq sharing
> + multiple rqs"] and this problem is more difficult to fix since the
> full cdb read is delayed to a common point further along the submit
> processing path. To detect a change in cbd[0] my current code would
> need to be altered to carry cdb[0] through to that common point. So
> is it worth it for such an old, deprecated and replaced interface??
> What cdb/user_permissions checking that is done, is done _after_
> the full cdb is read. So trying to get around a user exclusion of
> say WRITE(10) by first using the first byte of READ(10), won't succeed.
> 
> Doug Gilbert
> 
Thanks for your explaination. I guess this patch should be dropped for
the above reasons.

Thanks
Gen

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

end of thread, other threads:[~2019-06-06  8:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-23  2:38 [PATCH] sg: Fix a double-fetch bug in drivers/scsi/sg.c Gen Zhang
2019-06-05  6:00 ` Jiri Slaby
2019-06-05 17:07   ` Douglas Gilbert
2019-06-06  8:00     ` Gen Zhang

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).