[PATCH] sg: fix a double-fetch bug in sg_write()

[PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

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;
 	/*
---

Re: [PATCH] sg: fix a double-fetch bug in sg_write()

[Jiri Slaby]

On 31. 05. 19, 3:27, 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;
>  	if (__copy_from_user(cmnd, buf, cmd_size))
>  		return -EFAULT;

You are sending the same patches like a broken machine. Please STOP this
and give people some time to actually review your patches! (Don't expect
replies in days.)

I already commented on this apparently broken one earlier...

thanks,
-- 
js
suse labs

Re: [PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

On Wed, Jun 05, 2019 at 08:41:11AM +0200, Jiri Slaby wrote:
> On 31. 05. 19, 3:27, 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;
> >  	if (__copy_from_user(cmnd, buf, cmd_size))
> >  		return -EFAULT;
> 
> You are sending the same patches like a broken machine. Please STOP this
> and give people some time to actually review your patches! (Don't expect
> replies in days.)
> 
Thanks for your reply. I resubmitted this one after 8-day-no-reply. I 
don't judge whether this is a short time period or not. I politely hope
that you can reply more kindly.

I am just a PhD candidate. All I did is submitting patches, discussing 
with maintainers in accordance with linux community rules for academic papers.

I guess that you might be busy person and hope that submitting patches 
didn't bother you.

Thanks
Gen
> I already commented on this apparently broken one earlier...
> 
> thanks,
> -- 
> js
> suse labs

Re: [PATCH] sg: fix a double-fetch bug in sg_write()

[Jiri Slaby]

On 05. 06. 19, 17:35, Gen Zhang wrote:
> On Wed, Jun 05, 2019 at 08:41:11AM +0200, Jiri Slaby wrote:
>> On 31. 05. 19, 3:27, 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;
>>>  	if (__copy_from_user(cmnd, buf, cmd_size))
>>>  		return -EFAULT;
>>
>> You are sending the same patches like a broken machine. Please STOP this
>> and give people some time to actually review your patches! (Don't expect
>> replies in days.)
>>
> Thanks for your reply. I resubmitted this one after 8-day-no-reply. I 
> don't judge whether this is a short time period or not. I politely hope
> that you can reply more kindly.

There is no reason to be offended. I am just asking you to wait a bit
more before reposting. 8 days is too few. My personal experience says to
give patches like these something close to a month, esp. during the
merge window. The issues are present for a long time, nobody hit them
during that timeframe, so there is no reason to haste.

> I am just a PhD candidate. All I did is submitting patches, discussing 
> with maintainers in accordance with linux community rules for academic papers.

Yes, despite I have no idea what "linux community rules for academic
papers" are.

> I guess that you might be busy person and hope that submitting patches 
> didn't bother you.

It does not bother me at all. Patches are welcome, but newcomers tend to
send new versions of patches (or reposts) too quickly. It then leads to
wasting time of people where one person comments on one version and the
others don't see it and reply to some other.

thanks,
-- 
js
suse labs

Re: [PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

On Thu, Jun 06, 2019 at 07:01:26AM +0200, Jiri Slaby wrote:
> On 05. 06. 19, 17:35, Gen Zhang wrote:
> > On Wed, Jun 05, 2019 at 08:41:11AM +0200, Jiri Slaby wrote:
> >> On 31. 05. 19, 3:27, 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;
> >>>  	if (__copy_from_user(cmnd, buf, cmd_size))
> >>>  		return -EFAULT;
> >>
> >> You are sending the same patches like a broken machine. Please STOP this
> >> and give people some time to actually review your patches! (Don't expect
> >> replies in days.)
> >>
> > Thanks for your reply. I resubmitted this one after 8-day-no-reply. I 
> > don't judge whether this is a short time period or not. I politely hope
> > that you can reply more kindly.
> 
> There is no reason to be offended. I am just asking you to wait a bit
> more before reposting. 8 days is too few. My personal experience says to
> give patches like these something close to a month, esp. during the
> merge window. The issues are present for a long time, nobody hit them
> during that timeframe, so there is no reason to haste.
Thanks for your reply, and I will keep this in mind.
> 
> > I am just a PhD candidate. All I did is submitting patches, discussing 
> > with maintainers in accordance with linux community rules for academic papers.
> 
> Yes, despite I have no idea what "linux community rules for academic
> papers" are.

I mean, these patches come from a research project prototype. Submitting
patches, and getting it applied can be demonstrated in the experiment 
part of research paper.

Thanks
Gen
> 
> > I guess that you might be busy person and hope that submitting patches 
> > didn't bother you.
> 
> It does not bother me at all. Patches are welcome, but newcomers tend to
> send new versions of patches (or reposts) too quickly. It then leads to
> wasting time of people where one person comments on one version and the
> others don't see it and reply to some other.
> 
> thanks,
> -- 
> js
> suse labs

[PATCH] sg: fix a double-fetch bug in sg_write()

[Gen Zhang]

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;
 	/*
---