On Tue, Jul 10, 2018 at 3:24 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> So /dev/sg really has serious issues. Not just the read/write part,
> but even the SG_IO part is broken right now (sg_new_write() actually
> does do blk_verify_command(), but only for read-only opens for some
> reason).
Looking more at this, it's even more broken than that.
Look at SCSI_IOCTL_SEND_COMMAND. It will do a sg_scsi_ioctl(), but
before that it does the same crazy sg_allow_access() case - and only
for read-only opens.
But that's *doubly* wrong, because
(a) it's racy, and does the command check on a local copy that might
not be the final one.
(b) it's pointless, because sg_scsi_ioctl() actually does the proper
command check on the final command as it was copied from user space.
And yes, sg_scsi_ioctl() itself has a slight race in that first it
reads the first byte of the command ("opcode") in order to look up the
size of the command, and then it reads the whole command - including
the first byte - again. So it has the same "read twice, use possibly
inconsistent data" issue, but the actual command that is checked is
the final one that matches the command that is actually submitted. So
all you can do is confuse "cmdlen" and then get timeouts and retry
attempts based on the "fist command" value, but the actually sent
command is fine.
I'm not even going to bother with the sg_scsi_ioctl() race, because
it's harmless, but the drivers/scsi/sg.c code is just pointless and
confusing and should be removed.
Something like the attached?
Please, can we try to at least just de-crapify some of this code?
There's that other "sg_allow_access()" that I also think is wrong and
should just use blk_verify_command() directly and even when opened for
read, but that whole "read or TYPE_SCANNER" is presumably some special
crazy hack for some odd SCSI device.
Linus
From f075dce66c6039bb97b762b592a6db6d79e4350a Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Date: Tue, 10 Jul 2018 17:02:17 -0700
Subject: [PATCH] scsi sg: remove incorrect scsi command checking logic
The SCSI_IOCTL_SEND_COMMAND ioctl has interesting scsi command
"security" checking.
If the file was opened read-only (but only in that case), it will
fetch the first byte of the command from user space, and do
"sg_allow_access()" on it. That, in turn, will check that
"blk_verify_command()" is ok with that command byte.
If that passes, it will then do call "sg_scsi_ioctl()" to execute
the command.
This is entirely nonsensical for several reasons.
It's nonsensical simply because it's racy: after it copies the command
byte from user mode to check it, user mode could just change the byte
before it is actually submitted later by "sg_scsi_ioctl()".
But it is nonsensical also because "sg_scsi_ioctl()" itself already does
blk_verify_command() on the command properly after it has been copied
from user space.
So it is an incorrect implementation of a pointless check. Remove it.
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
---
drivers/scsi/sg.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index cd2fdac000c9..4f3450793273 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1103,15 +1103,6 @@ sg_ioctl(struct file *filp, unsigned int cmd_in, unsigned long arg)
case SCSI_IOCTL_SEND_COMMAND:
if (atomic_read(&sdp->detaching))
return -ENODEV;
- if (read_only) {
- unsigned char opcode = WRITE_6;
- Scsi_Ioctl_Command __user *siocp = p;
-
- if (copy_from_user(&opcode, siocp->data, 1))
- return -EFAULT;
- if (sg_allow_access(filp, &opcode))
- return -EPERM;
- }
return sg_scsi_ioctl(sdp->device->request_queue, NULL, filp->f_mode, p);
case SG_SET_DEBUG:
result = get_user(val, ip);
--
2.18.0.132.g95eda3d86
