[GIT PULL] SCSI fixes for 4.18-rc3

[GIT PULL] SCSI fixes for 4.18-rc3

[James Bottomley]

This is two minor bug fixes (aacraid, target) and a fix for a potential
exploit in the way sg handles teardown.

The patch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git scsi-fixes

The short changelog is:

David Disseldorp (1):
      scsi: target: Fix truncated PR-in ReadKeys response

Jann Horn (1):
      scsi: sg: mitigate read/write abuse

Raghava Aditya Renukunta (1):
      scsi: aacraid: Fix PD performance regression over incorrect qd being set

And the diffstat:

 drivers/scsi/aacraid/aachba.c   | 15 +++++++--------
 drivers/scsi/sg.c               | 42 +++++++++++++++++++++++++++++++++++++++--
 drivers/target/target_core_pr.c | 15 ++++++++++-----
 3 files changed, 57 insertions(+), 15 deletions(-)

With full diff below.

James

---

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index a9831bd37a73..a57f3a7d4748 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -1974,7 +1974,6 @@ static void aac_set_safw_attr_all_targets(struct aac_dev *dev)
 	u32 lun_count, nexus;
 	u32 i, bus, target;
 	u8 expose_flag, attribs;
-	u8 devtype;
 
 	lun_count = aac_get_safw_phys_lun_count(dev);
 
@@ -1992,23 +1991,23 @@ static void aac_set_safw_attr_all_targets(struct aac_dev *dev)
 			continue;
 
 		if (expose_flag != 0) {
-			devtype = AAC_DEVTYPE_RAID_MEMBER;
-			goto update_devtype;
+			dev->hba_map[bus][target].devtype =
+				AAC_DEVTYPE_RAID_MEMBER;
+			continue;
 		}
 
 		if (nexus != 0 && (attribs & 8)) {
-			devtype = AAC_DEVTYPE_NATIVE_RAW;
+			dev->hba_map[bus][target].devtype =
+				AAC_DEVTYPE_NATIVE_RAW;
 			dev->hba_map[bus][target].rmw_nexus =
 					nexus;
 		} else
-			devtype = AAC_DEVTYPE_ARC_RAW;
+			dev->hba_map[bus][target].devtype =
+				AAC_DEVTYPE_ARC_RAW;
 
 		dev->hba_map[bus][target].scan_counter = dev->scan_counter;
 
 		aac_set_safw_target_qd(dev, bus, target);
-
-update_devtype:
-		dev->hba_map[bus][target].devtype = devtype;
 	}
 }
 
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 53ae52dbff84..cd2fdac000c9 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -51,6 +51,7 @@ static int sg_version_num = 30536;	/* 2 digits for each component */
 #include <linux/atomic.h>
 #include <linux/ratelimit.h>
 #include <linux/uio.h>
+#include <linux/cred.h> /* for sg_check_file_access() */
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -209,6 +210,33 @@ static void sg_device_destroy(struct kref *kref);
 	sdev_prefix_printk(prefix, (sdp)->device,		\
 			   (sdp)->disk->disk_name, fmt, ##a)
 
+/*
+ * The SCSI interfaces that use read() and write() as an asynchronous variant of
+ * ioctl(..., SG_IO, ...) are fundamentally unsafe, since there are lots of ways
+ * to trigger read() and write() calls from various contexts with elevated
+ * privileges. This can lead to kernel memory corruption (e.g. if these
+ * interfaces are called through splice()) and privilege escalation inside
+ * userspace (e.g. if a process with access to such a device passes a file
+ * descriptor to a SUID binary as stdin/stdout/stderr).
+ *
+ * This function provides protection for the legacy API by restricting the
+ * calling context.
+ */
+static int sg_check_file_access(struct file *filp, const char *caller)
+{
+	if (filp->f_cred != current_real_cred()) {
+		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
+			caller, task_tgid_vnr(current), current->comm);
+		return -EPERM;
+	}
+	if (uaccess_kernel()) {
+		pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
+			caller, task_tgid_vnr(current), current->comm);
+		return -EACCES;
+	}
+	return 0;
+}
+
 static int sg_allow_access(struct file *filp, unsigned char *cmd)
 {
 	struct sg_fd *sfp = filp->private_data;
@@ -393,6 +421,14 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 	struct sg_header *old_hdr = NULL;
 	int retval = 0;
 
+	/*
+	 * This could cause a response to be stranded. Close the associated
+	 * file descriptor to free up any resources being held.
+	 */
+	retval = sg_check_file_access(filp, __func__);
+	if (retval)
+		return retval;
+
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
 	SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp,
@@ -580,9 +616,11 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	struct sg_header old_hdr;
 	sg_io_hdr_t *hp;
 	unsigned char cmnd[SG_MAX_CDB_SIZE];
+	int retval;
 
-	if (unlikely(uaccess_kernel()))
-		return -EINVAL;
+	retval = sg_check_file_access(filp, __func__);
+	if (retval)
+		return retval;
 
 	if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
 		return -ENXIO;
diff --git a/drivers/target/target_core_pr.c b/drivers/target/target_core_pr.c
index 01ac306131c1..10db5656fd5d 100644
--- a/drivers/target/target_core_pr.c
+++ b/drivers/target/target_core_pr.c
@@ -3727,11 +3727,16 @@ core_scsi3_pri_read_keys(struct se_cmd *cmd)
 		 * Check for overflow of 8byte PRI READ_KEYS payload and
 		 * next reservation key list descriptor.
 		 */
-		if ((add_len + 8) > (cmd->data_length - 8))
-			break;
-
-		put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
-		off += 8;
+		if (off + 8 <= cmd->data_length) {
+			put_unaligned_be64(pr_reg->pr_res_key, &buf[off]);
+			off += 8;
+		}
+		/*
+		 * SPC5r17: 6.16.2 READ KEYS service action
+		 * The ADDITIONAL LENGTH field indicates the number of bytes in
+		 * the Reservation key list. The contents of the ADDITIONAL
+		 * LENGTH field are not altered based on the allocation length
+		 */
 		add_len += 8;
 	}
 	spin_unlock(&dev->t10_pr.registration_lock);

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

On Fri, Jul 6, 2018 at 2:38 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> This is two minor bug fixes (aacraid, target) and a fix for a potential
> exploit in the way sg handles teardown.

Gahh. Is this where the IB people got their insane model from, using
read/write as ioclt replacements?

We have that ib_safe_file_access() hack for IB for this exact reason.

Who actually does direct read/write to /dev/sg? Could we perhaps just
add a config option to disable it entirely? If you want to send a SCSI
command, why don't you just use SG_IO? That's the only thing that
actually works on most devices (ie anything that isn't /dev/sg, and
nobody sane uses /dev/sg any more).

              Linus

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

On Fri, Jul 6, 2018 at 7:31 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Who actually does direct read/write to /dev/sg? Could we perhaps just
> add a config option to disable it entirely?

On the IB side, the argument was that there was some crazy binary-only
vendor management code that really wanted to use this completely crazy
interface.

I also think that the warnings are dubious. I'd rather add a
deprecation warning to the whole "read/write to /dev/sg" itself, and
then do what we did for ib_safe_file_access(), which was to just have
the permission checks.

It's not like a normal person should have access to /dev/sg to begin
with. So it's not like you can open /dev/sg0 and then try to fool a
suid program into doing the actual IO.

I'd hope.

Maybe I'm wrong, and there's some crazy "let's make /dev/sg available
to normal users" setup out there somewhere. At least for me, /dev/sg
isn't accessible to normal people:

  [torvalds@i7 linux]$ cat /dev/sg0
  cat: /dev/sg0: Permission denied

but maybe some distro decided that everybody should have direct device access..

Jann?

                 Linus

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

On Fri, Jul 6, 2018 at 7:39 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'd rather add a deprecation warning to the whole "read/write
> to /dev/sg" itself

In the meantime, I've pulled this, but do wonder why we actually allow
that crazy read/write that doesn't even work for any other models (ie
I guarantee you that cdrom writers etc don't use that interface,
because SG_IO is the only thing that works on most hardware).

              Linus

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Jann Horn]

On Sat, Jul 7, 2018 at 4:39 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Jul 6, 2018 at 7:31 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > Who actually does direct read/write to /dev/sg? Could we perhaps just
> > add a config option to disable it entirely?
>
> On the IB side, the argument was that there was some crazy binary-only
> vendor management code that really wanted to use this completely crazy
> interface.
>
> I also think that the warnings are dubious. I'd rather add a
> deprecation warning to the whole "read/write to /dev/sg" itself, and
> then do what we did for ib_safe_file_access(), which was to just have
> the permission checks.

I guess we could try that...
Douglas Gilbert said that the read/write interface was the original
one, so there might be some users left... but the two hits I just
looked at on Debian codesearch seem to have switched to the SG_IO
ioctl already, using the read/write API as fallback only.

> It's not like a normal person should have access to /dev/sg to begin
> with. So it's not like you can open /dev/sg0 and then try to fool a
> suid program into doing the actual IO.
>
> I'd hope.
>
> Maybe I'm wrong, and there's some crazy "let's make /dev/sg available
> to normal users" setup out there somewhere. At least for me, /dev/sg
> isn't accessible to normal people:
>
>   [torvalds@i7 linux]$ cat /dev/sg0
>   cat: /dev/sg0: Permission denied
>
> but maybe some distro decided that everybody should have direct device access..

Al Viro pointed out that some distros grant access to CD devices to
non-root. I've checked in a Debian VM and a Ubuntu VM, and there,
/dev/sg0 is mode 0660, group "cdrom". That's not "everybody", but it's
not just root. At least in the Ubuntu VM, the user account that the
system installer created has access to "cdrom", but accounts that I
create afterwards using the settings UI don't get access to that
group. Unless other distros are more lax, it's probably not a big
issue?

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

On Fri, Jul 6, 2018 at 8:08 PM Jann Horn <jannh@google.com> wrote:
>
>
> I guess we could try that...
> Douglas Gilbert said that the read/write interface was the original
> one, so there might be some users left...

I think it's the "original" interface as in "predating Linux, and
maybe early 90's".

There are *very* few things that do direct SCSI commands in the first
place. The traditional thing was for things like low-level formatting
and for reading audio CD data or burning CD/DVD's.

Yes, in _theory_ there are other things, but they are much more rare.

ATA made /dev/sg* irrelevant for CD/DVDs, and you literally *had* to
use SG_IO to do it.

And if it's anything even remotely generic (ie "this can work on SATA
or NVMe"), again, only SG_IO has a chance in hell of working.

I could imagine that yes, there's some crazy vendor disk array
configuration tool that still uses read/write since it only works with
some very particular hardware, but even then SG_IO should just be much
more convenient than the odd read/write interface.

> > but maybe some distro decided that everybody should have direct device access..
>
> Al Viro pointed out that some distros grant access to CD devices to
> non-root. I've checked in a Debian VM and a Ubuntu VM, and there,
> /dev/sg0 is mode 0660, group "cdrom". That's not "everybody", but it's
> not just root. At least in the Ubuntu VM, the user account that the
> system installer created has access to "cdrom", but accounts that I
> create afterwards using the settings UI don't get access to that
> group. Unless other distros are more lax, it's probably not a big
> issue?

I suspect Fedora is similar. I have a USB storage device, which is why
I have /dev/sg0, and it's in the "disk" group. I'm not part of it even
as the primary user, so..

Anyway, cdrom devices are definitely *not* a reason for using
/dev/sg*, exactly because no cdrom burner would ever use anything but
SG_IO, because it they did, they'd not work in 99% of all cases during
the big reign of cheap ATA CD/DVD drives.

                Linus

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[James Bottomley]

On Fri, 2018-07-06 at 19:48 -0700, Linus Torvalds wrote:
> On Fri, Jul 6, 2018 at 7:39 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > 
> > I'd rather add a deprecation warning to the whole "read/write
> > to /dev/sg" itself
> 
> In the meantime, I've pulled this, but do wonder why we actually
> allow that crazy read/write that doesn't even work for any other
> models (ie I guarantee you that cdrom writers etc don't use that
> interface, because SG_IO is the only thing that works on most
> hardware).

We did discuss removing the r/w interface, but, as you say, it's been
around for ages so it's not clear what regressions would surface if we
did.  It's mostly root only (with certain distro exceptions), so the
consensus for a short term fix was to make sure it couldn't be
exploited.  Long term we'll absolutely look into removing it.

The argument I've seen for the old interface is userspace programs that
want multiple outstanding commands in the old event driven single
threaded model (with SG_IO you need one thread for each command) but if
you asked me to name any, I couldn't, so perhaps they're all gone by
now.

James

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Fri, Jul 6, 2018 at 10:22 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> We did discuss removing the r/w interface, but, as you say, it's been
> around for ages so it's not clear what regressions would surface if we
> did.

So since nobody else followed up on this, the attached patch is what I
was thinking of just committing.

It removes the warnings from the access check, and just puts them
(unconditionally) at the top of the read/write function instead.

Hmm?

                 Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1955 bytes --]

 drivers/scsi/sg.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index cd2fdac000c9..09325b8fbc9f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -222,18 +222,12 @@ static void sg_device_destroy(struct kref *kref);
  * This function provides protection for the legacy API by restricting the
  * calling context.
  */
-static int sg_check_file_access(struct file *filp, const char *caller)
+static int sg_check_file_access(struct file *filp)
 {
-	if (filp->f_cred != current_real_cred()) {
-		pr_err_once("%s: process %d (%s) changed security contexts after opening file descriptor, this is not allowed.\n",
-			caller, task_tgid_vnr(current), current->comm);
+	if (filp->f_cred != current_real_cred())
 		return -EPERM;
-	}
-	if (uaccess_kernel()) {
-		pr_err_once("%s: process %d (%s) called from kernel context, this is not allowed.\n",
-			caller, task_tgid_vnr(current), current->comm);
+	if (uaccess_kernel())
 		return -EACCES;
-	}
 	return 0;
 }
 
@@ -421,11 +415,14 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
 	struct sg_header *old_hdr = NULL;
 	int retval = 0;
 
+	pr_err_once("process %d (%s) does direct read on /dev/sg",
+		task_tgid_vnr(current), current->comm);
+
 	/*
 	 * This could cause a response to be stranded. Close the associated
 	 * file descriptor to free up any resources being held.
 	 */
-	retval = sg_check_file_access(filp, __func__);
+	retval = sg_check_file_access(filp);
 	if (retval)
 		return retval;
 
@@ -618,7 +615,10 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
 	unsigned char cmnd[SG_MAX_CDB_SIZE];
 	int retval;
 
-	retval = sg_check_file_access(filp, __func__);
+	pr_err_once("process %d (%s) does direct write on /dev/sg",
+		task_tgid_vnr(current), current->comm);
+
+	retval = sg_check_file_access(filp);
 	if (retval)
 		return retval;
 

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Jann Horn]

On Mon, Jul 9, 2018 at 5:41 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Jul 6, 2018 at 10:22 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> > We did discuss removing the r/w interface, but, as you say, it's been
> > around for ages so it's not clear what regressions would surface if we
> > did.
>
> So since nobody else followed up on this, the attached patch is what I
> was thinking of just committing.
>
> It removes the warnings from the access check, and just puts them
> (unconditionally) at the top of the read/write function instead.
>
> Hmm?

Random unimportant nitpickery:

AFAICS it does mean that if two processes use /dev/sg* - the first one
in a way that passes sg_check_file_access(), the second one in a way
that gets blocked for whatever reason -, the pr_err_once() will fire
for the process that's working and not fire for the one that got
blocked. But if nobody should be using that interface anyway, I guess
that's not a practical concern.

Also, the device is called /dev/sg%d with %d being sdp->index.

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

On Tue, Jul 10, 2018 at 10:49 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> > Also, the device is called /dev/sg%d with %d being sdp->index.
>
> I guess I could make it at least say that, although it's not like "one
> of them would be ok, but /dev/sd3 is right out".

Actually, I don't want to after all, because I don't even have 'spd'
available until later when we might have already returned ENXIO. I'd
rather have the warning right at the top of the function.

               Linus

Re: Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Tony Battersby]

At my job (https://www.cybernetics.com/), I use the write()/read()
interface to the SCSI generic driver for access to tape drives and tape
medium changers.  For example, the write()/read() interface is useful
for implementing RAID-like functionality for tape drives since a single
thread can send commands to multiple tape drives at once and poll() for
command completion.  We have a lot of code invested in this interface,
so it would be a huge pain for us if it were removed.  But in our case,
everything runs as root (as the firmware of an embedded storage
appliance), so extra permission checks should be OK.

Tony Battersby
Cybernetics

Re: Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

On Tue, Jul 10, 2018 at 2:53 PM Tony Battersby <tonyb@cybernetics.com> wrote:
>
> At my job (https://www.cybernetics.com/), I use the write()/read()
> interface to the SCSI generic driver for access to tape drives and tape
> medium changers.  For example, the write()/read() interface is useful
> for implementing RAID-like functionality for tape drives since a single
> thread can send commands to multiple tape drives at once and poll() for
> command completion.

Ugh.

> We have a lot of code invested in this interface,
> so it would be a huge pain for us if it were removed.  But in our case,
> everything runs as root (as the firmware of an embedded storage
> appliance), so extra permission checks should be OK.

I wonder if we could make the warning literally about not having
CAP_SYS_RAWIO on the file descriptor.

Because the /dev/sg interfaces don't even do the kind of command
filtering that the SG_IO code does.

The SG_IO code (well, at least the block/scsi_ioctl.c code - again
/dev/sg does NOT get it right!) actually has a whitelist of commands
that have known behavior, and that can be done by normal users when
you open a device.

To actually do _arbitrary_ commands, you have to have CAP_SYS_RAWIO.
And there we check the _current_ capabilities (not the file descriptor
one), because an ioctl isn't something you just fool a random suid
program into doing for you.

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).

                 Linus

Re: Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

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

[-- Attachment #2: 0001-scsi-sg-remove-incorrect-scsi-command-checking-logic.patch --]
[-- Type: text/x-patch, Size: 1964 bytes --]

From f075dce66c6039bb97b762b592a6db6d79e4350a Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
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@linux-foundation.org>
---
 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

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Christoph Hellwig]

On Tue, Jul 10, 2018 at 05:53:18PM -0400, Tony Battersby wrote:
> At my job (https://www.cybernetics.com/), I use the write()/read()
> interface to the SCSI generic driver for access to tape drives and tape
> medium changers.  For example, the write()/read() interface is useful
> for implementing RAID-like functionality for tape drives since a single
> thread can send commands to multiple tape drives at once and poll() for
> command completion.  We have a lot of code invested in this interface,
> so it would be a huge pain for us if it were removed.  But in our case,
> everything runs as root (as the firmware of an embedded storage
> appliance), so extra permission checks should be OK.

Do you just use read/write on /dev/sg or also on /dev/bsg?  Because
I started a discussion to kill the read/write support for the latter
even before Linus brought it up here..  (and we have the same fix
pending for /dev/bsg)

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Tony Battersby]

On 07/11/2018 02:45 AM, Christoph Hellwig wrote:
> On Tue, Jul 10, 2018 at 05:53:18PM -0400, Tony Battersby wrote:
>> At my job (https://www.cybernetics.com/), I use the write()/read()
>> interface to the SCSI generic driver for access to tape drives and tape
>> medium changers.  For example, the write()/read() interface is useful
>> for implementing RAID-like functionality for tape drives since a single
>> thread can send commands to multiple tape drives at once and poll() for
>> command completion.  We have a lot of code invested in this interface,
>> so it would be a huge pain for us if it were removed.  But in our case,
>> everything runs as root (as the firmware of an embedded storage
>> appliance), so extra permission checks should be OK.
> Do you just use read/write on /dev/sg or also on /dev/bsg?  Because
> I started a discussion to kill the read/write support for the latter
> even before Linus brought it up here..  (and we have the same fix
> pending for /dev/bsg)
>
The read/write interface on /dev/bsg is impossible to use safely because
the list of completed commands is per-device (bd->done_list) rather than
per-fd like it is with /dev/sg.  So if program A and program B are both
using the write/read interface on the same bsg device, then their
command responses will get mixed up, and program A will read() some
command results from program B and vice versa.  So no, I don't use
read/write on /dev/bsg.  From a security standpoint, it should
definitely be fixed or removed.

Another issue with the read/write interface of /dev/bsg was this:

[PATCH] [SCSI] bsg: fix unkillable I/O wait deadlock with scsi-mq
https://marc.info/?l=linux-scsi&m=142367231311098&w=2

My similar patch to sg.c was accepted as commit 7568615c1054 ("sg: fix
unkillable I/O wait deadlock with scsi-mq"), but my bsg patch was never
applied.  I do not know if the problem still exists in the current
kernel or if some other change to scsi-mq has fixed it.  I do have a
forward-port of the patch to 4.16, but it no longer applies to 4.17.

---

A while ago there was a commit that broke the read/write interface of
/dev/sg (but not SG_IO), and some other people complained in the
following bugzilla entry, indicating that there are other users also:

https://bugzilla.kernel.org/show_bug.cgi?id=198081

The commit that broke it was:
109bade9c625 ("scsi: sg: use standard lists for sg_requests")

The commit that fixed it was:
48ae8484e9fc ("scsi: sg: don't return bogus Sg_requests")
(sg_get_rq_mark() is called only from sg_read(), so if the patch fixed
the problem that people were complaining about in bugzilla, then that
indicates that they are using the read/write interface also)

See my explanation here:
https://marc.info/?l=linux-scsi&m=152227354106242
(basically, the commit that broke it got backported to -stable, but the
fix didn't, so various -stable kernels were broken for a while, and
people complained)

Tony Battersby
Cybernetics

Re: [GIT PULL] SCSI fixes for 4.18-rc3

[Jann Horn]

On Tue, Jul 10, 2018 at 2:41 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Jul 6, 2018 at 10:22 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> >
> > We did discuss removing the r/w interface, but, as you say, it's been
> > around for ages so it's not clear what regressions would surface if we
> > did.
>
> So since nobody else followed up on this, the attached patch is what I
> was thinking of just committing.
>
> It removes the warnings from the access check, and just puts them
> (unconditionally) at the top of the read/write function instead.

Minor issue:

+ pr_err_once("process %d (%s) does direct read on /dev/sg",
+ task_tgid_vnr(current), current->comm);
[...]
+ pr_err_once("process %d (%s) does direct write on /dev/sg",
+ task_tgid_vnr(current), current->comm);

printk wants a newline at the end of the message, otherwise the
message hangs until the next message is printed.