Messages in this thread Patch in this message | | | Date | Sat, 12 Sep 2020 08:47:57 +0100 | From | Christoph Hellwig <> | Subject | Re: [PATCH 3/3] scsi: megaraid_sas: simplify compat_ioctl handling |
| |
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.
| |