SCSI fixes for 4.18-rc3

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

---

index a9831bd37a73..a57f3a7d4748 100644
@@ -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;
 	}
 }
 

index 01ac306131c1..10db5656fd5d 100644
@@ -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);

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

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

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

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

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

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

index cd2fdac000c9..09325b8fbc9f 100644
@@ -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;
 

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.

Right. I don't expect it to trigger at all, and the "working" program should be fixed too, because I want for us to be able to just remove that idiotic direct access thing.

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

Linus

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

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

Ugh.

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

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 <[email protected]>
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.

---
 drivers/scsi/sg.c | 9 ---------
 1 file changed, 9 deletions(-)

index cd2fdac000c9..4f3450793273 100644
@@ -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

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

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.

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.

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

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