Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 12 Sep 2020 08:47:57 +0100
From:   Christoph Hellwig <hch@infradead.org>
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Kashyap Desai <kashyap.desai@broadcom.com>,
        Sumit Saxena <sumit.saxena@broadcom.com>,
        Shivasharan S <shivasharan.srikanteshwara@broadcom.com>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        hch@infradead.org, Anand Lodnoor <anand.lodnoor@broadcom.com>,
        Chandrakanth Patil <chandrakanth.patil@broadcom.com>,
        Hannes Reinecke <hare@suse.de>, megaraidlinux.pdl@broadcom.com,
        linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] scsi: megaraid_sas: simplify compat_ioctl handling
Message-ID: <20200912074757.GA6688@infradead.org>
References: <20200908213715.3553098-1-arnd@arndb.de>
 <20200908213715.3553098-3-arnd@arndb.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200908213715.3553098-3-arnd@arndb.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.