[PATCH] scsi: ensure the header peeked does not change in the actual message

[PATCH] scsi: ensure the header peeked does not change in the actual message

[Meng Xu]

In sg_scsi_ioctl(), the header of the userspace buffer, sic->data, is
fetched twice from the userspace. The first fetch is used to peek at the
opcode and derive cmdlen while the second fetch copies the whole message.
However, the userspace memory backing opcode can change across fetches
which means that the corresponding opcode field in req->cmd could be
different from what is fetched in for the first time (and so is the
derived cmdlen).

Whether this double-fetch situation is a security critical bug depends
on how req->cmd will be used later. However, given that it is hard to
enumerate all the possible use cases, a safer way is to ensure that
the peeked header does not change after the second fetch.

This patch enforces that the opcode field in req->cmd is the same as what
is fetched in originally.

Signed-off-by: Meng Xu <mengxu.gatech@gmail.com>
---
 block/scsi_ioctl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 7440de4..971044d 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -466,6 +466,12 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
 	if (copy_from_user(req->cmd, sic->data, cmdlen))
 		goto error;
 
+	/*
+	 * override the request header (opcode) to make sure that it matches
+	 * the first fetch from sic->data 
+	 */
+	*((unsigned int *)req->cmd) = opcode;
+
 	if (in_len && copy_from_user(buffer, sic->data + cmdlen, in_len))
 		goto error;
 
-- 
2.7.4

Re: [PATCH] scsi: ensure the header peeked does not change in the actual message

[Christoph Hellwig]

> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index 7440de4..971044d 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -466,6 +466,12 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
>  	if (copy_from_user(req->cmd, sic->data, cmdlen))
>  		goto error;
>  
> +	/*
> +	 * override the request header (opcode) to make sure that it matches
> +	 * the first fetch from sic->data 
> +	 */
> +	*((unsigned int *)req->cmd) = opcode;
> +
>  	if (in_len && copy_from_user(buffer, sic->data + cmdlen, in_len))

NAK.

Just don't copy the byte twice.  E.g. change things to not copy
the first byte again.

Re: [PATCH] scsi: ensure the header peeked does not change in the actual message

[Meng Xu]

Hi Christoph,

By saying not copying the byte twice, did you mean
copy_from_user(req->cmd, sic->data + sizeof(opcode), cmdlen - 
sizeof(opcode)) ?

Does it affect the how req->cmd will be used later?
If no, I'll submit another patch as instructed.

Best Regards,
Meng

On 09/19/2017 12:01 PM, Christoph Hellwig wrote:
>> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
>> index 7440de4..971044d 100644
>> --- a/block/scsi_ioctl.c
>> +++ b/block/scsi_ioctl.c
>> @@ -466,6 +466,12 @@ int sg_scsi_ioctl(struct request_queue *q, struct gendisk *disk, fmode_t mode,
>>   	if (copy_from_user(req->cmd, sic->data, cmdlen))
>>   		goto error;
>>   
>> +	/*
>> +	 * override the request header (opcode) to make sure that it matches
>> +	 * the first fetch from sic->data
>> +	 */
>> +	*((unsigned int *)req->cmd) = opcode;
>> +
>>   	if (in_len && copy_from_user(buffer, sic->data + cmdlen, in_len))
> NAK.
>
> Just don't copy the byte twice.  E.g. change things to not copy
> the first byte again.
>

Re: [PATCH] scsi: ensure the header peeked does not change in the actual message

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 12:15:57PM -0400, Meng Xu wrote:
> Hi Christoph,
> 
> By saying not copying the byte twice, did you mean
> copy_from_user(req->cmd, sic->data + sizeof(opcode), cmdlen -
> sizeof(opcode)) ?
> 
> Does it affect the how req->cmd will be used later?
> If no, I'll submit another patch as instructed.

No, do something like:

	req->cmd[0] = opcode;
   	if (copy_from_user(req->cmd + 1, sic->data, cmdlen - 1))
   		goto error;