The use of compat_alloc_user_space() can be easily replaced by
handling compat arguments in the regular handler, and this will
make it work for big-endian kernels as well, which at the moment
get an invalid indirect pointer argument.
Calling aac_ioctl() instead of aac_compat_do_ioctl() means the
compat and native code paths behave the same way again, which
they stopped when the adapter health check was added only
in the native function.
Fixes: 572ee53a9bad ("scsi: aacraid: check adapter health")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/scsi/aacraid/commctrl.c | 20 +++++++++--
drivers/scsi/aacraid/linit.c | 61 ++-------------------------------
2 files changed, 20 insertions(+), 61 deletions(-)
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index 59e82a832042..6d6f72d68164 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -25,6 +25,7 @@
#include <linux/completion.h>
#include <linux/dma-mapping.h>
#include <linux/blkdev.h>
+#include <linux/compat.h>
#include <linux/delay.h> /* ssleep prototype */
#include <linux/kthread.h>
#include <linux/uaccess.h>
@@ -243,8 +244,23 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg)
struct list_head * entry;
unsigned long flags;
- if(copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl)))
- return -EFAULT;
+ if (in_compat_syscall()) {
+ struct compat_fib_ioctl {
+ u32 fibctx;
+ s32 wait;
+ compat_uptr_t fib;
+ } cf;
+
+ if (copy_from_user(&f, arg, sizeof(struct compat_fib_ioctl)))
+ return -EFAULT;
+
+ f.fibctx = cf.fibctx;
+ f.wait = cf.wait;
+ f.fib = compat_ptr(cf.fib);
+ } else {
+ if (copy_from_user(&f, arg, sizeof(struct fib_ioctl)))
+ return -EFAULT;
+ }
/*
* Verify that the HANDLE passed in was a valid AdapterFibContext
*
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 8588da0a0655..8c0f55845138 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1182,63 +1182,6 @@ static long aac_cfg_ioctl(struct file *file,
return aac_do_ioctl(aac, cmd, (void __user *)arg);
}
-#ifdef CONFIG_COMPAT
-static long aac_compat_do_ioctl(struct aac_dev *dev, unsigned cmd, unsigned long arg)
-{
- long ret;
- switch (cmd) {
- case FSACTL_MINIPORT_REV_CHECK:
- case FSACTL_SENDFIB:
- case FSACTL_OPEN_GET_ADAPTER_FIB:
- case FSACTL_CLOSE_GET_ADAPTER_FIB:
- case FSACTL_SEND_RAW_SRB:
- case FSACTL_GET_PCI_INFO:
- case FSACTL_QUERY_DISK:
- case FSACTL_DELETE_DISK:
- case FSACTL_FORCE_DELETE_DISK:
- case FSACTL_GET_CONTAINERS:
- case FSACTL_SEND_LARGE_FIB:
- ret = aac_do_ioctl(dev, cmd, (void __user *)arg);
- break;
-
- case FSACTL_GET_NEXT_ADAPTER_FIB: {
- struct fib_ioctl __user *f;
-
- f = compat_alloc_user_space(sizeof(*f));
- ret = 0;
- if (clear_user(f, sizeof(*f)))
- ret = -EFAULT;
- if (copy_in_user(f, (void __user *)arg, sizeof(struct fib_ioctl) - sizeof(u32)))
- ret = -EFAULT;
- if (!ret)
- ret = aac_do_ioctl(dev, cmd, f);
- break;
- }
-
- default:
- ret = -ENOIOCTLCMD;
- break;
- }
- return ret;
-}
-
-static int aac_compat_ioctl(struct scsi_device *sdev, unsigned int cmd,
- void __user *arg)
-{
- struct aac_dev *dev = (struct aac_dev *)sdev->host->hostdata;
- if (!capable(CAP_SYS_RAWIO))
- return -EPERM;
- return aac_compat_do_ioctl(dev, cmd, (unsigned long)arg);
-}
-
-static long aac_compat_cfg_ioctl(struct file *file, unsigned cmd, unsigned long arg)
-{
- if (!capable(CAP_SYS_RAWIO))
- return -EPERM;
- return aac_compat_do_ioctl(file->private_data, cmd, arg);
-}
-#endif
-
static ssize_t aac_show_model(struct device *device,
struct device_attribute *attr, char *buf)
{
@@ -1523,7 +1466,7 @@ static const struct file_operations aac_cfg_fops = {
.owner = THIS_MODULE,
.unlocked_ioctl = aac_cfg_ioctl,
#ifdef CONFIG_COMPAT
- .compat_ioctl = aac_compat_cfg_ioctl,
+ .compat_ioctl = aac_cfg_ioctl,
#endif
.open = aac_cfg_open,
.llseek = noop_llseek,
@@ -1536,7 +1479,7 @@ static struct scsi_host_template aac_driver_template = {
.info = aac_info,
.ioctl = aac_ioctl,
#ifdef CONFIG_COMPAT
- .compat_ioctl = aac_compat_ioctl,
+ .compat_ioctl = aac_ioctl,
#endif
.queuecommand = aac_queuecommand,
.bios_param = aac_biosparm,
--
2.27.0
It sounds unwise to let user space pass an unchecked 32-bit
offset into a kernel structure in an ioctl. This is an unsigned
variable, so checking the upper bound for the size of the structure
it points into is sufficient to avoid data corruption, but as
the pointer might also be unaligned, it has to be written carefully
as well.
While I stumbled over this problem by reading the code, I did not
continue checking the function for further problems like it.
Cc: [email protected]
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/scsi/megaraid/megaraid_sas_base.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 861f7140f52e..c3de69f3bee8 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -8095,7 +8095,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
int error = 0, i;
void *sense = NULL;
dma_addr_t sense_handle;
- unsigned long *sense_ptr;
+ void *sense_ptr;
u32 opcode = 0;
int ret = DCMD_SUCCESS;
@@ -8218,6 +8218,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
}
if (ioc->sense_len) {
+ /* make sure the pointer is part of the frame */
+ if (ioc->sense_off > (sizeof(union megasas_frame) - sizeof(__le64))) {
+ error = -EINVAL;
+ goto out;
+ }
+
sense = dma_alloc_coherent(&instance->pdev->dev, ioc->sense_len,
&sense_handle, GFP_KERNEL);
if (!sense) {
@@ -8225,12 +8231,11 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
goto out;
}
- sense_ptr =
- (unsigned long *) ((unsigned long)cmd->frame + ioc->sense_off);
+ sense_ptr = (void *)cmd->frame + ioc->sense_off;
if (instance->consistent_mask_64bit)
- *sense_ptr = cpu_to_le64(sense_handle);
+ put_unaligned_le64(sense_handle, sense_ptr);
else
- *sense_ptr = cpu_to_le32(sense_handle);
+ put_unaligned_le32(sense_handle, sense_ptr);
}
/*
--
2.27.0
There have been several attempts to fix serious problems
in the compat handling in megasas_mgmt_compat_ioctl_fw(),
and it also uses the compat_alloc_user_space() function.
Folding the compat handling into the regular ioctl
function with in_compat_syscall() simplifies it a lot and
avoids some of the remaining problems:
- missing handling of unaligned pointers
- overflowing the ioc->frame.raw array from
invalid input
- compat_alloc_user_space()
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/scsi/megaraid/megaraid_sas_base.c | 123 ++++++++++------------
1 file changed, 56 insertions(+), 67 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index c3de69f3bee8..601dab468823 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -8331,6 +8331,56 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
return error;
}
+static struct megasas_iocpacket *megasas_compat_iocpacket_get_user(void __user *arg)
+{
+ int err = -EFAULT;
+#ifdef CONFIG_COMPAT
+ struct megasas_iocpacket *ioc;
+ struct compat_megasas_iocpacket __user *cioc = arg;
+ int i;
+
+ ioc = kzalloc(sizeof(*ioc), GFP_KERNEL);
+ if (copy_from_user(ioc, arg,
+ offsetof(struct megasas_iocpacket, frame) + 128))
+ goto out;
+
+ /*
+ * The sense_ptr is used in megasas_mgmt_fw_ioctl only when
+ * sense_len is not null, so prepare the 64bit value under
+ * the same condition.
+ */
+ if (ioc->sense_len) {
+ compat_uptr_t *sense_ioc_ptr;
+ void __user *sense_cioc;
+
+ /* make sure the pointer is inside of frame.raw */
+ if (ioc->sense_off >
+ (sizeof(ioc->frame.raw) - sizeof(void __user*))) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ sense_ioc_ptr = (compat_uptr_t *)&ioc->frame.raw[ioc->sense_off];
+ sense_cioc = compat_ptr(get_unaligned(sense_ioc_ptr));
+ put_unaligned((unsigned long)sense_cioc, (void **)sense_ioc_ptr);
+ }
+
+ for (i = 0; i < MAX_IOCTL_SGE; i++) {
+ compat_uptr_t iov_base;
+ if (get_user(iov_base, &cioc->sgl[i].iov_base) ||
+ get_user(ioc->sgl[i].iov_len, &cioc->sgl[i].iov_len)) {
+ goto out;
+ }
+ ioc->sgl[i].iov_base = compat_ptr(iov_base);
+ }
+
+ return ioc;
+out:
+ kfree(ioc);
+#endif
+ return ERR_PTR(err);
+}
+
static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg)
{
struct megasas_iocpacket __user *user_ioc =
@@ -8339,7 +8389,11 @@ static int megasas_mgmt_ioctl_fw(struct file *file, unsigned long arg)
struct megasas_instance *instance;
int error;
- ioc = memdup_user(user_ioc, sizeof(*ioc));
+ if (in_compat_syscall())
+ ioc = megasas_compat_iocpacket_get_user(user_ioc);
+ else
+ ioc = memdup_user(user_ioc, sizeof(struct megasas_iocpacket));
+
if (IS_ERR(ioc))
return PTR_ERR(ioc);
@@ -8444,78 +8498,13 @@ megasas_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
}
#ifdef CONFIG_COMPAT
-static int megasas_mgmt_compat_ioctl_fw(struct file *file, unsigned long arg)
-{
- struct compat_megasas_iocpacket __user *cioc =
- (struct compat_megasas_iocpacket __user *)arg;
- struct megasas_iocpacket __user *ioc =
- compat_alloc_user_space(sizeof(struct megasas_iocpacket));
- int i;
- int error = 0;
- compat_uptr_t ptr;
- u32 local_sense_off;
- u32 local_sense_len;
- u32 user_sense_off;
-
- if (clear_user(ioc, sizeof(*ioc)))
- return -EFAULT;
-
- if (copy_in_user(&ioc->host_no, &cioc->host_no, sizeof(u16)) ||
- copy_in_user(&ioc->sgl_off, &cioc->sgl_off, sizeof(u32)) ||
- copy_in_user(&ioc->sense_off, &cioc->sense_off, sizeof(u32)) ||
- copy_in_user(&ioc->sense_len, &cioc->sense_len, sizeof(u32)) ||
- copy_in_user(ioc->frame.raw, cioc->frame.raw, 128) ||
- copy_in_user(&ioc->sge_count, &cioc->sge_count, sizeof(u32)))
- return -EFAULT;
-
- /*
- * The sense_ptr is used in megasas_mgmt_fw_ioctl only when
- * sense_len is not null, so prepare the 64bit value under
- * the same condition.
- */
- if (get_user(local_sense_off, &ioc->sense_off) ||
- get_user(local_sense_len, &ioc->sense_len) ||
- get_user(user_sense_off, &cioc->sense_off))
- return -EFAULT;
-
- if (local_sense_off != user_sense_off)
- return -EINVAL;
-
- if (local_sense_len) {
- void __user **sense_ioc_ptr =
- (void __user **)((u8 *)((unsigned long)&ioc->frame.raw) + local_sense_off);
- compat_uptr_t *sense_cioc_ptr =
- (compat_uptr_t *)(((unsigned long)&cioc->frame.raw) + user_sense_off);
- if (get_user(ptr, sense_cioc_ptr) ||
- put_user(compat_ptr(ptr), sense_ioc_ptr))
- return -EFAULT;
- }
-
- for (i = 0; i < MAX_IOCTL_SGE; i++) {
- if (get_user(ptr, &cioc->sgl[i].iov_base) ||
- put_user(compat_ptr(ptr), &ioc->sgl[i].iov_base) ||
- copy_in_user(&ioc->sgl[i].iov_len,
- &cioc->sgl[i].iov_len, sizeof(compat_size_t)))
- return -EFAULT;
- }
-
- error = megasas_mgmt_ioctl_fw(file, (unsigned long)ioc);
-
- if (copy_in_user(&cioc->frame.hdr.cmd_status,
- &ioc->frame.hdr.cmd_status, sizeof(u8))) {
- printk(KERN_DEBUG "megasas: error copy_in_user cmd_status\n");
- return -EFAULT;
- }
- return error;
-}
-
static long
megasas_mgmt_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
switch (cmd) {
case MEGASAS_IOC_FIRMWARE32:
- return megasas_mgmt_compat_ioctl_fw(file, arg);
+ return megasas_mgmt_ioctl_fw(file, arg);
case MEGASAS_IOC_GET_AEN:
return megasas_mgmt_ioctl_aen(file, arg);
}
--
2.27.0
On Tue, Sep 08, 2020 at 11:36:21PM +0200, Arnd Bergmann wrote:
> @@ -243,8 +244,23 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg)
> struct list_head * entry;
> unsigned long flags;
>
> - if(copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl)))
> - return -EFAULT;
> + if (in_compat_syscall()) {
> + struct compat_fib_ioctl {
> + u32 fibctx;
> + s32 wait;
> + compat_uptr_t fib;
> + } cf;
I find the struct declaration deep down in the function a little
annoying.
But otherwise this looks good;
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Sep 08, 2020 at 11:36:22PM +0200, Arnd Bergmann wrote:
> It sounds unwise to let user space pass an unchecked 32-bit
> offset into a kernel structure in an ioctl. This is an unsigned
> variable, so checking the upper bound for the size of the structure
> it points into is sufficient to avoid data corruption, but as
> the pointer might also be unaligned, it has to be written carefully
> as well.
>
> While I stumbled over this problem by reading the code, I did not
> continue checking the function for further problems like it.
Oh, yikes!
>
> Cc: [email protected]
What about a Fixes tag instead?
> if (ioc->sense_len) {
> + /* make sure the pointer is part of the frame */
> + if (ioc->sense_off > (sizeof(union megasas_frame) - sizeof(__le64))) {
No need for the inner braces and please avoid over 80 char lines.
Otherwise looks good:
Reviewed-by: Christoph Hellwig <[email protected]>
On Tue, Sep 08, 2020 at 11:36:23PM +0200, Arnd Bergmann wrote:
> There have been several attempts to fix serious problems
> in the compat handling in megasas_mgmt_compat_ioctl_fw(),
> and it also uses the compat_alloc_user_space() function.
I just looked into this a few weeks ago but didn't get that far..
> +static struct megasas_iocpacket *megasas_compat_iocpacket_get_user(void __user *arg)
Pointlessly long line.
> +{
> + int err = -EFAULT;
> +#ifdef CONFIG_COMPAT
I find the ifdef inside the function a little weird. Doing it in the
caller would be a little less bad. What I ended up doing in my
unfinished patch was to move the compat handling into a new
megaraid_sas_compat.c file, so we'd always get the prototypes in a
header, but given that all the calls are eliminated for the !COMPAT
case we'd avoid ifdefs entirely, but having that file for a single
function is also rather silly.
> + struct megasas_iocpacket *ioc;
> + struct compat_megasas_iocpacket __user *cioc = arg;
> + int i;
> +
> + ioc = kzalloc(sizeof(*ioc), GFP_KERNEL);
Missing NULL check here.
> + if (copy_from_user(ioc, arg,
> + offsetof(struct megasas_iocpacket, frame) + 128))
> + goto out;
the 128 here while copied from the original code should probably be
replaced with a sizeof(frame->raw).
> + if (ioc->sense_len) {
> + compat_uptr_t *sense_ioc_ptr;
> + void __user *sense_cioc;
> +
> + /* make sure the pointer is inside of frame.raw */
> + if (ioc->sense_off >
> + (sizeof(ioc->frame.raw) - sizeof(void __user*))) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> + sense_ioc_ptr = (compat_uptr_t *)&ioc->frame.raw[ioc->sense_off];
> + sense_cioc = compat_ptr(get_unaligned(sense_ioc_ptr));
> + put_unaligned((unsigned long)sense_cioc, (void **)sense_ioc_ptr);
I think we should really handle this where the sense point is set up.
This is the untested hunk I had:
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 48fad675b5ed02..c3ddcfce86df50 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -8231,7 +8231,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
goto out;
}
- sense_ptr = (void *)cmd->frame + ioc->sense_off;
+ if (in_compat_syscall())
+ sense_ptr = compat_ptr((uintptr_t)cmd->frame) +
+ ioc->sense_off;
+ else
+ sense_ptr = (void *)cmd->frame + ioc->sense_off;
+
if (instance->consistent_mask_64bit)
put_unaligned_le64(sense_handle, sense_ptr);
else
The same might make sense for the iovecs, but I didn't get to that
yet..
> static long
> megasas_mgmt_compat_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> switch (cmd) {
> case MEGASAS_IOC_FIRMWARE32:
> - return megasas_mgmt_compat_ioctl_fw(file, arg);
> + return megasas_mgmt_ioctl_fw(file, arg);
> case MEGASAS_IOC_GET_AEN:
> return megasas_mgmt_ioctl_aen(file, arg);
> }
We should be able to kill off megasas_mgmt_compat_ioctl entirely now.
On Sat, Sep 12, 2020 at 9:48 AM Christoph Hellwig <[email protected]> wrote:
>
> On Tue, Sep 08, 2020 at 11:36:23PM +0200, Arnd Bergmann wrote:
> > There have been several attempts to fix serious problems
> > in the compat handling in megasas_mgmt_compat_ioctl_fw(),
> > and it also uses the compat_alloc_user_space() function.
>
> I just looked into this a few weeks ago but didn't get that far..
Ok. To make sure we don't needlessly duplicate work, here are the
ones I have looked at over the past few days:
drivers/scsi/aacraid/linit.c: f = compat_alloc_user_space(sizeof(*f));
drivers/scsi/megaraid/megaraid_sas_base.c:
compat_alloc_user_space(sizeof(struct megasas_iocpacket));
Sent out here.
drivers/video/fbdev/core/fbmem.c: cmap = compat_alloc_user_space(sizeof(*cmap));
fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct
fs_quota_stat));
kernel/kexec.c: ksegments = compat_alloc_user_space(nr_segments * sizeof(out));
mm/mempolicy.c: nm = compat_alloc_user_space(alloc_size);
mm/mempolicy.c: nm = compat_alloc_user_space(alloc_size);
mm/mempolicy.c: nm = compat_alloc_user_space(alloc_size);
mm/mempolicy.c: old = compat_alloc_user_space(new_nodes ? size * 2 : size);
mm/mempolicy.c: new = compat_alloc_user_space(size);
mm/migrate.c: pages = compat_alloc_user_space(nr_pages * sizeof(void *));
net/socket.c: rxnfc = compat_alloc_user_space(buf_size);
net/socket.c: uifr = compat_alloc_user_space(sizeof(*uifr));
sound/core/control_compat.c: data = compat_alloc_user_space(sizeof(*data));
sound/core/hwdep_compat.c: dst = compat_alloc_user_space(sizeof(*dst));
drivers/video/fbdev/sbuslib.c: struct fbcursor __user *p =
compat_alloc_user_space(sizeof(*p));
I have patches pending for all the above, but not sent out.
drivers/media/v4l2-core/v4l2-compat-ioctl32.c: *new_p64 =
compat_alloc_user_space(size + aux_space);
Currently looking at this. I have a plan but it's not as easy.
drivers/video/fbdev/sbuslib.c: struct fbcmap __user *p =
compat_alloc_user_space(sizeof(*p));
I played around with this one but could not come up with a solution that
is better than the current one. It's only used on sparc64 though, so we
might just leave the interface on that architecture until someone else
takes care of it.
drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c: karg =
compat_alloc_user_space(
Had a brief look but did not investigate further, it's complicated.
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
compat_alloc_user_space(sizeof(*args));
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
compat_alloc_user_space(sizeof(*args) +
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
compat_alloc_user_space(sizeof(*args));
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
compat_alloc_user_space(sizeof(*args) +
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
compat_alloc_user_space(sizeof(*args));
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
compat_alloc_user_space(sizeof(*args));
Should not be too hard, but I have not looked in detail.
> > +static struct megasas_iocpacket *megasas_compat_iocpacket_get_user(void __user *arg)
>
> Pointlessly long line.
Ok
> > +{
> > + int err = -EFAULT;
> > +#ifdef CONFIG_COMPAT
>
> I find the ifdef inside the function a little weird. Doing it in the
> caller would be a little less bad. What I ended up doing in my
> unfinished patch was to move the compat handling into a new
> megaraid_sas_compat.c file, so we'd always get the prototypes in a
> header, but given that all the calls are eliminated for the !COMPAT
> case we'd avoid ifdefs entirely, but having that file for a single
> function is also rather silly.
My first attempt was to have no #ifdef at all, but that would require
moving "struct compat_iovec" definition outside of the CONFIG_COMPAT
check as well. That might be the cleanest though.
The reason I ended up with the #ifdef inside of the function was that
otherwise I either needed one around the caller and one around
the function definition, or an empty inline in the #else path.
> > + struct megasas_iocpacket *ioc;
> > + struct compat_megasas_iocpacket __user *cioc = arg;
> > + int i;
> > +
> > + ioc = kzalloc(sizeof(*ioc), GFP_KERNEL);
>
> Missing NULL check here.
ok.
> > + if (copy_from_user(ioc, arg,
> > + offsetof(struct megasas_iocpacket, frame) + 128))
> > + goto out;
>
> the 128 here while copied from the original code should probably be
> replaced with a sizeof(frame->raw).
ok.
> > + if (ioc->sense_len) {
> > + compat_uptr_t *sense_ioc_ptr;
> > + void __user *sense_cioc;
> > +
> > + /* make sure the pointer is inside of frame.raw */
> > + if (ioc->sense_off >
> > + (sizeof(ioc->frame.raw) - sizeof(void __user*))) {
> > + err = -EINVAL;
> > + goto out;
> > + }
> > +
> > + sense_ioc_ptr = (compat_uptr_t *)&ioc->frame.raw[ioc->sense_off];
> > + sense_cioc = compat_ptr(get_unaligned(sense_ioc_ptr));
> > + put_unaligned((unsigned long)sense_cioc, (void **)sense_ioc_ptr);
>
> I think we should really handle this where the sense point is set up.
> This is the untested hunk I had:
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 48fad675b5ed02..c3ddcfce86df50 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -8231,7 +8231,12 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
> goto out;
> }
>
> - sense_ptr = (void *)cmd->frame + ioc->sense_off;
> + if (in_compat_syscall())
> + sense_ptr = compat_ptr((uintptr_t)cmd->frame) +
> + ioc->sense_off;
> + else
> + sense_ptr = (void *)cmd->frame + ioc->sense_off;
> +
> if (instance->consistent_mask_64bit)
> put_unaligned_le64(sense_handle, sense_ptr);
> else
>
> The same might make sense for the iovecs, but I didn't get to that
> yet..
I think you got the wrong one there, the code above is where the
dma address gets stored in the in-kernel copy of the sense data
based on the user space offset. Conceptually it does make sense
though and would end up looking something like
if (ioc->sense_len) {
/*
* sense_ptr points to the location that has the user
* sense buffer address
*/
sense_ptr = (void *)ioc->frame.raw + ioc->sense_off;
if (in_compat_syscall())
uptr = compat_ptr(get_unaligned(u32 *)sense_ptr);
else
uptr = get_unaligned((void __user **)sense_ptr);
if (copy_to_user(uptr, sense, ioc->sense_len)) {
> > static long
> > megasas_mgmt_compat_ioctl(struct file *file, unsigned int cmd,
> > unsigned long arg)
> > {
> > switch (cmd) {
> > case MEGASAS_IOC_FIRMWARE32:
> > - return megasas_mgmt_compat_ioctl_fw(file, arg);
> > + return megasas_mgmt_ioctl_fw(file, arg);
> > case MEGASAS_IOC_GET_AEN:
> > return megasas_mgmt_ioctl_aen(file, arg);
> > }
>
> We should be able to kill off megasas_mgmt_compat_ioctl entirely now.
I tried that, but there is still one difference because one of them uses
MEGASAS_IOC_FIRMWARE while the other one uses
MEGASAS_IOC_FIRMWARE32. It would be possible to have
a common handler that always handles both command codes.
I tried to avoid changing the behavior that way though.
Arnd
On Sat, Sep 12, 2020 at 9:20 AM Christoph Hellwig <[email protected]> wrote:
> On Tue, Sep 08, 2020 at 11:36:22PM +0200, Arnd Bergmann wrote:
> > Cc: [email protected]
>
> What about a Fixes tag instead?
Sure, I can add that. It's been broken since 2.6.15 though, when the driver was
initially merged.
Arnd
On Sat, Sep 12, 2020 at 02:49:05PM +0200, Arnd Bergmann wrote:
> fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
> fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
> fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct
> fs_quota_stat));
I sent this out a while ago, an Al has it in a branch, but not in
linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.quota-compat
> drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c: karg =
> compat_alloc_user_space(
>
> Had a brief look but did not investigate further, it's complicated.
>
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> compat_alloc_user_space(sizeof(*args));
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> compat_alloc_user_space(sizeof(*args) +
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> compat_alloc_user_space(sizeof(*args));
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> compat_alloc_user_space(sizeof(*args) +
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> compat_alloc_user_space(sizeof(*args));
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> compat_alloc_user_space(sizeof(*args));
>
> Should not be too hard, but I have not looked in detail.
We do not have to care about staging drivers when removing interfaces.
But to be nice you probably ping the maintainers to see what they can
do.
I also have the mount side handles in this branch which I need to rebase
and submit:
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/mount-cleanups
> I think you got the wrong one there, the code above is where the
> dma address gets stored in the in-kernel copy of the sense data
> based on the user space offset. Conceptually it does make sense
> though and would end up looking something like
>
> if (ioc->sense_len) {
> /*
> * sense_ptr points to the location that has the user
> * sense buffer address
> */
> sense_ptr = (void *)ioc->frame.raw + ioc->sense_off;
> if (in_compat_syscall())
> uptr = compat_ptr(get_unaligned(u32 *)sense_ptr);
> else
> uptr = get_unaligned((void __user **)sense_ptr);
>
> if (copy_to_user(uptr, sense, ioc->sense_len)) {
Indeed. As said, I had started on the change and gave up pretty quickly
:)
> I tried that, but there is still one difference because one of them uses
> MEGASAS_IOC_FIRMWARE while the other one uses
> MEGASAS_IOC_FIRMWARE32. It would be possible to have
> a common handler that always handles both command codes.
> I tried to avoid changing the behavior that way though.
Ok.
On Sun, Sep 13, 2020 at 8:50 AM Christoph Hellwig <[email protected]> wrote:
>
> On Sat, Sep 12, 2020 at 02:49:05PM +0200, Arnd Bergmann wrote:
> > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
> > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
> > fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct
> > fs_quota_stat));
>
> I sent this out a while ago, an Al has it in a branch, but not in
> linux-next:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.quota-compat
Nice! Aside from already being queued, your patch is also nicer than
my version, and it makes it trivial to fix it for arm oabi as well by adding
#ifdef CONFIG_OABI_COMPAT
#define compat_need_64bit_alignment_fixup in_oabi_syscall
#endif
to arch/arm/include/asm/compat.h
I had considered fixing that case for arch/arm as well but it ended up being
harder to do in my version.
> > drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.c: karg =
> > compat_alloc_user_space(
> >
> > Had a brief look but did not investigate further, it's complicated.
> >
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> > compat_alloc_user_space(sizeof(*args));
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> > compat_alloc_user_space(sizeof(*args) +
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> > compat_alloc_user_space(sizeof(*args));
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> > compat_alloc_user_space(sizeof(*args) +
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> > compat_alloc_user_space(sizeof(*args));
> > drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c: args =
> > compat_alloc_user_space(sizeof(*args));
> >
> > Should not be too hard, but I have not looked in detail.
>
> We do not have to care about staging drivers when removing interfaces.
> But to be nice you probably ping the maintainers to see what they can
> do.
Right. As both of these are architecture specific, I also considered moving
the compat_alloc_user_space() and copy_in_user() definitions for the
respective architectures into those drivers and adding the removal
into the TODO files.
> I also have the mount side handles in this branch which I need to rebase
> and submit:
>
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/mount-cleanups
I think I had done an almost identical patch for sys_mount() last year
and forgotten about it. Again, yours is slightly better ;-)
Arnd
On Sat, Sep 12, 2020 at 9:09 AM Christoph Hellwig <[email protected]> wrote:
>
> On Tue, Sep 08, 2020 at 11:36:21PM +0200, Arnd Bergmann wrote:
> > @@ -243,8 +244,23 @@ static int next_getadapter_fib(struct aac_dev * dev, void __user *arg)
> > struct list_head * entry;
> > unsigned long flags;
> >
> > - if(copy_from_user((void *)&f, arg, sizeof(struct fib_ioctl)))
> > - return -EFAULT;
> > + if (in_compat_syscall()) {
> > + struct compat_fib_ioctl {
> > + u32 fibctx;
> > + s32 wait;
> > + compat_uptr_t fib;
> > + } cf;
>
> I find the struct declaration deep down in the function a little
> annoying.
>
> But otherwise this looks good;
>
> Reviewed-by: Christoph Hellwig <[email protected]>
I got back to these patches now and moved the struct definition, plus
fixed a typo I noticed while doing so. Thanks!
Arnd
On Sun, Sep 13, 2020 at 1:46 PM Arnd Bergmann <[email protected]> wrote:
>
> On Sun, Sep 13, 2020 at 8:50 AM Christoph Hellwig <[email protected]> wrote:
> >
> > On Sat, Sep 12, 2020 at 02:49:05PM +0200, Arnd Bergmann wrote:
> > > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
> > > fs/quota/compat.c: dqblk = compat_alloc_user_space(sizeof(struct if_dqblk));
> > > fs/quota/compat.c: fsqstat = compat_alloc_user_space(sizeof(struct
> > > fs_quota_stat));
> >
> > I sent this out a while ago, an Al has it in a branch, but not in
> > linux-next:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git/log/?h=work.quota-compat
>
> Nice! Aside from already being queued, your patch is also nicer than
> my version, and it makes it trivial to fix it for arm oabi as well by adding
>
> #ifdef CONFIG_OABI_COMPAT
> #define compat_need_64bit_alignment_fixup in_oabi_syscall
> #endif
>
> to arch/arm/include/asm/compat.h
>
> I had considered fixing that case for arch/arm as well but it ended up being
> harder to do in my version.
Unfortunately, the commit b902bfb3f0e "arm64: stop using <asm/compat.h>
directly" seems to introduce a circular header file inclusion between
linux/compat.h and asm/stat.h, breaking arm64 compilation.
Moving the compat_u64/compat_s64 definitions to include/asm-generic/compat.h
works fine though.
Arnd
On Thu, Sep 17, 2020 at 04:55:49PM +0200, Arnd Bergmann wrote:
> Unfortunately, the commit b902bfb3f0e "arm64: stop using <asm/compat.h>
> directly" seems to introduce a circular header file inclusion between
> linux/compat.h and asm/stat.h, breaking arm64 compilation.
>
> Moving the compat_u64/compat_s64 definitions to include/asm-generic/compat.h
> works fine though.
I posted a version doing exactly that a few hours ago.
On Tue, Sep 08, 2020 at 11:36:22PM +0200, Arnd Bergmann wrote:
> It sounds unwise to let user space pass an unchecked 32-bit
> offset into a kernel structure in an ioctl. This is an unsigned
> variable, so checking the upper bound for the size of the structure
> it points into is sufficient to avoid data corruption, but as
> the pointer might also be unaligned, it has to be written carefully
> as well.
>
> While I stumbled over this problem by reading the code, I did not
> continue checking the function for further problems like it.
Sorry for replying to an ancient thread, but this patch just recently
made it into 5.10.3 and has caused unintended consequences. On Dell
servers with PERC RAID controllers, booting 5.10.3+ with this patch
causes a PCI parity error. Specifically:
Event Message: A PCI parity error was detected on a component at bus 0 device 5 function 0.
Severity: Critical
Message ID: PCI1308
I reverted this single patch and the errors went away.
Thoughts?
Phil Oester
On Thu, Dec 31, 2020 at 1:15 AM Phil Oester <[email protected]> wrote:
>
> On Tue, Sep 08, 2020 at 11:36:22PM +0200, Arnd Bergmann wrote:
> > It sounds unwise to let user space pass an unchecked 32-bit
> > offset into a kernel structure in an ioctl. This is an unsigned
> > variable, so checking the upper bound for the size of the structure
> > it points into is sufficient to avoid data corruption, but as
> > the pointer might also be unaligned, it has to be written carefully
> > as well.
> >
> > While I stumbled over this problem by reading the code, I did not
> > continue checking the function for further problems like it.
>
> Sorry for replying to an ancient thread, but this patch just recently
> made it into 5.10.3 and has caused unintended consequences. On Dell
> servers with PERC RAID controllers, booting 5.10.3+ with this patch
> causes a PCI parity error. Specifically:
>
> Event Message: A PCI parity error was detected on a component at bus 0 device 5 function 0.
> Severity: Critical
> Message ID: PCI1308
>
> I reverted this single patch and the errors went away.
>
> Thoughts?
Thank you for the report and bisecting the issue, and sorry this broke
your system!
Fortunately, the patch is fairly small, so there are only a limited number
of things that could go wrong. I haven't tried to analyze that message,
but I have two ideas:
a) The added ioc->sense_off check gets triggered and the code relies
on the data being written outside of the structure
b) the address actually needs to always be written as a 64-bit value
regardless of the instance->consistent_mask_64bit flag, as the
driver did before. This looked like it was done in error.
Can you try the patch below instead of the revert and see if that
resolves the regression, and if it triggers the warning message I
add?
Arnd
diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
b/drivers/scsi/megaraid/megaraid_sas_base.c
index 6e4bf05c6d77..248063a4148b 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -8194,8 +8194,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
/* make sure the pointer is part of the frame */
if (ioc->sense_off >
(sizeof(union megasas_frame) - sizeof(__le64))) {
- error = -EINVAL;
- goto out;
+ pr_warn("possible out of bounds access offset
%d\n", ioc->sense_off);
}
sense = dma_alloc_coherent(&instance->pdev->dev, ioc->sense_len,
@@ -8209,7 +8208,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
if (instance->consistent_mask_64bit)
put_unaligned_le64(sense_handle, sense_ptr);
else
- put_unaligned_le32(sense_handle, sense_ptr);
+ put_unaligned_le64(sense_handle, sense_ptr);
}
/*
On Sun, 2021-01-03 at 17:26 +0100, Arnd Bergmann wrote:
[...]
> @@ -8209,7 +8208,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance
> *instance,
> if (instance->consistent_mask_64bit)
> put_unaligned_le64(sense_handle, sense_ptr);
> else
> - put_unaligned_le32(sense_handle, sense_ptr);
> + put_unaligned_le64(sense_handle, sense_ptr);
> }
This hunk can't be right. It effectively means removing the if.
However, the if is needed because sense_handle is a dma_addr_t which
can be either 32 or 64 bit. What about changing the if to
if (sizeof(dma_addr_t) == 8)
instead?
James
On Sun, 2021-01-03 at 19:49 +0100, Arnd Bergmann wrote:
> On Sun, Jan 3, 2021 at 6:00 PM James Bottomley <[email protected]>
> wrote:
> > On Sun, 2021-01-03 at 17:26 +0100, Arnd Bergmann wrote:
> > [...]
> > > @@ -8209,7 +8208,7 @@ megasas_mgmt_fw_ioctl(struct
> > > megasas_instance
> > > *instance,
> > > if (instance->consistent_mask_64bit)
> > > put_unaligned_le64(sense_handle,
> > > sense_ptr);
> > > else
> > > - put_unaligned_le32(sense_handle,
> > > sense_ptr);
> > > + put_unaligned_le64(sense_handle,
> > > sense_ptr);
> > > }
> >
> > This hunk can't be right. It effectively means removing the if.
>
> I'm just trying to restore the state before the regression introduced
> in my 381d34e376e3 ("scsi: megaraid_sas: Check user-provided
> offsets").
>
> The old code always stored 'sizeof(long)' bytes into sense_ptr,
> regardless of instance->consistent_mask_64bit, but it would truncate
> the address to 32 bit if that was cleared. This was clearly bogus
> and I tried to make it do something more meaningful, only storing
> 8 bytes into the structure if it was configured for 64-bit DMA,
> regardless of the capabilities of the kernel.
Heh, well, all this depends on how the firmware interprets the pointer,
for which we don't seem to have a manual. Instinct tells me the flag
MFI_FRAME_SENSE64 is what does this and that's conditioned on the same
if clause 100 lines above this, so the fix your proposing would still
seem to be wrong, because I think when that flag is not set, the device
expects the sense pointer to be 32 bit.
> > However, the if is needed because sense_handle is a dma_addr_t
> > which can be either 32 or 64 bit. What about changing the if to
> >
> > if (sizeof(dma_addr_t) == 8)
> >
> > instead?
>
> That would not be useful either, the device surely does not care
> if the kernel supports 64-bit DMA. What we'd really need here is
> someone with access to the interface specifications to see how
> many bytes should be stored in the structure. I suspect always
> storing 64 bits (as my patch does) is correct, and would send a
> proper patch to remove the if() if Phil confirms that my test
> patch fixes the regression.
Well, as I said above, I'm speculating the device does what we tell it,
and whether to use 32 or 64 bits for the sense pointer definitely seems
to be a flag the driver controls ... we really need someone with access
to the programming manual to tell us if this speculation is accurate,
though.
James
On Sun, Jan 3, 2021 at 6:00 PM James Bottomley <[email protected]> wrote:
> On Sun, 2021-01-03 at 17:26 +0100, Arnd Bergmann wrote:
> [...]
> > @@ -8209,7 +8208,7 @@ megasas_mgmt_fw_ioctl(struct megasas_instance
> > *instance,
> > if (instance->consistent_mask_64bit)
> > put_unaligned_le64(sense_handle, sense_ptr);
> > else
> > - put_unaligned_le32(sense_handle, sense_ptr);
> > + put_unaligned_le64(sense_handle, sense_ptr);
> > }
>
> This hunk can't be right. It effectively means removing the if.
I'm just trying to restore the state before the regression introduced
in my 381d34e376e3 ("scsi: megaraid_sas: Check user-provided offsets").
The old code always stored 'sizeof(long)' bytes into sense_ptr,
regardless of instance->consistent_mask_64bit, but it would truncate
the address to 32 bit if that was cleared. This was clearly bogus
and I tried to make it do something more meaningful, only storing
8 bytes into the structure if it was configured for 64-bit DMA, regardless
of the capabilities of the kernel.
> However, the if is needed because sense_handle is a dma_addr_t which
> can be either 32 or 64 bit. What about changing the if to
>
> if (sizeof(dma_addr_t) == 8)
>
> instead?
That would not be useful either, the device surely does not care
if the kernel supports 64-bit DMA. What we'd really need here is
someone with access to the interface specifications to see how
many bytes should be stored in the structure. I suspect always
storing 64 bits (as my patch does) is correct, and would send a
proper patch to remove the if() if Phil confirms that my test
patch fixes the regression.
Arnd
On Sun, Jan 03, 2021 at 05:26:29PM +0100, Arnd Bergmann wrote:
> Thank you for the report and bisecting the issue, and sorry this broke
> your system!
>
> Fortunately, the patch is fairly small, so there are only a limited number
> of things that could go wrong. I haven't tried to analyze that message,
> but I have two ideas:
>
> a) The added ioc->sense_off check gets triggered and the code relies
> on the data being written outside of the structure
>
> b) the address actually needs to always be written as a 64-bit value
> regardless of the instance->consistent_mask_64bit flag, as the
> driver did before. This looked like it was done in error.
>
> Can you try the patch below instead of the revert and see if that
> resolves the regression, and if it triggers the warning message I
> add?
Thanks Arnd, I tried your patch and it resolves the regression. It does not
trigger the warning message you added.
Phil
On Mon, Jan 4, 2021 at 6:48 PM Phil Oester <[email protected]> wrote:
>
> On Sun, Jan 03, 2021 at 05:26:29PM +0100, Arnd Bergmann wrote:
> > Thank you for the report and bisecting the issue, and sorry this broke
> > your system!
> >
> > Fortunately, the patch is fairly small, so there are only a limited number
> > of things that could go wrong. I haven't tried to analyze that message,
> > but I have two ideas:
> >
> > a) The added ioc->sense_off check gets triggered and the code relies
> > on the data being written outside of the structure
> >
> > b) the address actually needs to always be written as a 64-bit value
> > regardless of the instance->consistent_mask_64bit flag, as the
> > driver did before. This looked like it was done in error.
> >
> > Can you try the patch below instead of the revert and see if that
> > resolves the regression, and if it triggers the warning message I
> > add?
>
> Thanks Arnd, I tried your patch and it resolves the regression. It does not
> trigger the warning message you added.
Ok, thanks for testing! That would mean the range check is correct,
but the sense pointer must indeed be treated as a 64-bit entity
regardless of instance->consistent_mask_64bit, or at least the
upper 32 bit must be zero when the flag is unset, rather than
the recycled previous value.
I'll send a proper fix shortly, it would be nice if you could give it
another spin, but the behavior should be the same as this patch.
Arnd