On Tue, Jul 10, 2018 at 3:24 PM Linus Torvalds
<torvalds@linux-foundation.org> 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