Skip to contentSkip to conversation

sg: fix dxferp in from_to case

PATCH Applied Douglas Gilbert 3 participants 4 messages Mar 3, 2016 – Mar 10, 2016

Subsystems (2)
lore
Share & tools
lore

Douglas Gilbert

author maintainer
#0 lore raw

PATCH 1/1 sg: fix dxferp in from_to case

This patch is in response to this report:
http://www.spinics.net/lists/linux-scsi/msg93456.html

One of the strange things that the original sg driver did was let the user provide both a data-out buffer (it followed the sg_header+cdb) _and_ specify a reply length greater than zero. What happened was that the user data-out buffer was copied into some kernel buffers and then the mid level was told a read type operation would take place with the data from the device overwriting the same kernel buffers. The user would then read those kernel buffers back into the user space.

From what I can tell, the above action was broken by a change in 2008 and syzkaller found that out recently.

ChangeLog:
make sure that a user space pointer is passed through when data follows the sg_header structure and command. Fix the abnormal case when a non-zero reply_len is also given.

drivers/scsi/sg.c +2 -1
index 5e82067..ae7d9bd 100644
@@ -652,7 +652,8 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	else
 		hp->dxfer_direction = (mxsize > 0) ? SG_DXFER_FROM_DEV : SG_DXFER_NONE;
 	hp->dxfer_len = mxsize;
-	if (hp->dxfer_direction == SG_DXFER_TO_DEV)
+	if ((hp->dxfer_direction == SG_DXFER_TO_DEV) ||
+	    (hp->dxfer_direction == SG_DXFER_TO_FROM_DEV))
 		hp->dxferp = (char __user *)buf + cmd_size;
 	else
 		hp->dxferp = NULL;
#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Doug" == Douglas Gilbert <[email protected]> writes:

Doug> This patch is in response to this report:
Doug> http://www.spinics.net/lists/linux-scsi/msg93456.html

Doug> One of the strange things that the original sg driver did was let Doug> the user provide both a data-out buffer (it followed the Doug> sg_header+cdb) _and_ specify a reply length greater than Doug> zero. What happened was that the user data-out buffer was copied Doug> into some kernel buffers and then the mid level was told a read Doug> type operation would take place with the data from the device Doug> overwriting the same kernel buffers. The user would then read Doug> those kernel buffers back into the user space.

Doug> From what I can tell, the above action was broken by a change in Doug> 2008 and syzkaller found that out recently.

Doug> ChangeLog: make sure that a user space pointer is passed
Doug> through when data follows the sg_header structure and command. Doug> Fix the abnormal case when a non-zero reply_len is also given.

Somebody please review.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
Reviewed-by on v1 1/1
#2 lore raw
In reply to Douglas Gilbert
Quoted 15 lines · Douglas Gilbert

This patch is in response to this report:

    http://www.spinics.net/lists/linux-scsi/msg93456.html

One of the strange things that the original sg driver did was let the user provide both a data-out buffer (it followed the sg_header+cdb) _and_ specify a reply length greater than zero. What happened was that the user data-out buffer was copied into some kernel buffers and then the mid level was told a read type operation would take place with the data from the device overwriting the same kernel buffers. The user would then read those kernel buffers back into the user space.

From what I can tell, the above action was broken by a change in
2008 and syzkaller found that out recently.

Quoted 7 lines

ChangeLog:
make sure that a user space pointer is passed through when data follows the sg_header structure and command. Fix the abnormal case when a non-zero reply_len is also given.

This looks correct to me. hp->dxferp used to be set unconditionally, but commit fad7f01e changed it to only be set in the SG_DXFER_TO_DEV case.

#3 lore raw
In reply to Douglas Gilbert
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Doug" == Douglas Gilbert <[email protected]> writes:

Doug> Make sure that a user space pointer is passed through when data Doug> follows the sg_header structure and command. Fix the abnormal Doug> case when a non-zero reply_len is also given.

Applied to 4.5/scsi-fixes.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering