scsi: fix a double-fetch bug in sg_write
diff mbox series

Message ID 20181225202427.69476-1-kjlu@umn.edu
State New
Headers show
Series
  • scsi: fix a double-fetch bug in sg_write
Related show

Commit Message

Kangjie Lu Dec. 25, 2018, 8:24 p.m. UTC
"opcode" has been copied in from user space and checked. We should not
copy it in again, which may have been modified by malicous
multi-threading user programs through race conditions. The fix uses the
opcode fetched in the first copy.

Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
 drivers/scsi/sg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Douglas Gilbert Dec. 26, 2018, 3:42 a.m. UTC | #1
On 2018-12-25 3:24 p.m., Kangjie Lu wrote:
> "opcode" has been copied in from user space and checked. We should not
> copy it in again, which may have been modified by malicous
> multi-threading user programs through race conditions. The fix uses the
> opcode fetched in the first copy.
> 
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
  Acked-by: Douglas Gilbert <dgilbert@interlog.com>

Also applied to my sg v4 driver code. The v1 and v2 interfaces (based on
struct sg_header) did not provide a command length field. The sg driver
needed to read the first byte of the command (the "opcode") to determine
the full command's length prior to actually reading it in full.

Hard to think of an example of an exploit based on this double read.

> ---
>   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 4dacbfffd113..41774e4f9508 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -686,7 +686,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,
>

Patch
diff mbox series

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 4dacbfffd113..41774e4f9508 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -686,7 +686,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,