| Message ID | 1530913134.3135.2.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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?
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
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
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);
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 ---