Commit 26b5b874 authored by Jann Horn's avatar Jann Horn Committed by Martin K. Petersen

scsi: sg: mitigate read/write abuse

As Al Viro noted in commit 128394ef ("sg_write()/bsg_write() is not fit
to be called under KERNEL_DS"), sg improperly accesses userspace memory
outside the provided buffer, permitting kernel memory corruption via
splice().  But it doesn't just do it on ->write(), also on ->read().

As a band-aid, make sure that the ->read() and ->write() handlers can not
be called in weird contexts (kernel context or credentials different from
file opener), like for ib_safe_file_access().

If someone needs to use these interfaces from different security contexts,
a new interface should be written that goes through the ->ioctl() handler.

I've mostly copypasted ib_safe_file_access() over as sg_safe_file_access()
because I couldn't find a good common header - please tell me if you know a
better way.

[mkp: s/_safe_/_check_/]

Fixes: 1da177e4 ("Linux-2.6.12-rc2")
Cc: <stable@vger.kernel.org>
Signed-off-by: default avatarJann Horn <jannh@google.com>
Acked-by: default avatarDouglas Gilbert <dgilbert@interlog.com>
Signed-off-by: default avatarMartin K. Petersen <martin.petersen@oracle.com>
parent 59b433c8
...@@ -51,6 +51,7 @@ static int sg_version_num = 30536; /* 2 digits for each component */ ...@@ -51,6 +51,7 @@ static int sg_version_num = 30536; /* 2 digits for each component */
#include <linux/atomic.h> #include <linux/atomic.h>
#include <linux/ratelimit.h> #include <linux/ratelimit.h>
#include <linux/uio.h> #include <linux/uio.h>
#include <linux/cred.h> /* for sg_check_file_access() */
#include "scsi.h" #include "scsi.h"
#include <scsi/scsi_dbg.h> #include <scsi/scsi_dbg.h>
...@@ -209,6 +210,33 @@ static void sg_device_destroy(struct kref *kref); ...@@ -209,6 +210,33 @@ static void sg_device_destroy(struct kref *kref);
sdev_prefix_printk(prefix, (sdp)->device, \ sdev_prefix_printk(prefix, (sdp)->device, \
(sdp)->disk->disk_name, fmt, ##a) (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) static int sg_allow_access(struct file *filp, unsigned char *cmd)
{ {
struct sg_fd *sfp = filp->private_data; 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) ...@@ -393,6 +421,14 @@ sg_read(struct file *filp, char __user *buf, size_t count, loff_t * ppos)
struct sg_header *old_hdr = NULL; struct sg_header *old_hdr = NULL;
int retval = 0; 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))) if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
return -ENXIO; return -ENXIO;
SCSI_LOG_TIMEOUT(3, sg_printk(KERN_INFO, sdp, 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) ...@@ -580,9 +616,11 @@ sg_write(struct file *filp, const char __user *buf, size_t count, loff_t * ppos)
struct sg_header old_hdr; struct sg_header old_hdr;
sg_io_hdr_t *hp; sg_io_hdr_t *hp;
unsigned char cmnd[SG_MAX_CDB_SIZE]; unsigned char cmnd[SG_MAX_CDB_SIZE];
int retval;
if (unlikely(uaccess_kernel())) retval = sg_check_file_access(filp, __func__);
return -EINVAL; if (retval)
return retval;
if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp))) if ((!(sfp = (Sg_fd *) filp->private_data)) || (!(sdp = sfp->parentdp)))
return -ENXIO; return -ENXIO;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment