Re: [PATCH 3/3] scsi: megaraid_sas: simplify compat_ioctl handling

Christoph Hellwig
2020-09-12T07:48:20Z

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.