| Message ID | CA+55aFwg-2GP4ASTdd1pusmZkF7c8AN9febVDCaioDxzYJSLfw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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.
On Tue, Jul 10, 2018 at 10:36 AM Jann Horn <jannh@google.com> wrote: > > 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. 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. > 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". Linus
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
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
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
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)
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
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.
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;