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.
