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

[Christoph Hellwig <hch@infradead.org>]

On Fri, Sep 18, 2020 at 02:15:43PM +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.
> 
> 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 <arnd@arndb.de>
> ---
> v2: address review comments from hch
> ---
>  drivers/scsi/megaraid/megaraid_sas.h      |   2 -
>  drivers/scsi/megaraid/megaraid_sas_base.c | 117 +++++++++-------------
>  include/linux/compat.h                    |  10 +-
>  3 files changed, 50 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/scsi/megaraid/megaraid_sas.h b/drivers/scsi/megaraid/megaraid_sas.h
> index 5e4137f10e0e..0f808d63580e 100644
> --- a/drivers/scsi/megaraid/megaraid_sas.h
> +++ b/drivers/scsi/megaraid/megaraid_sas.h
> @@ -2605,7 +2605,6 @@ struct megasas_aen {
>  	u32 class_locale_word;
>  } __attribute__ ((packed));
>  
> -#ifdef CONFIG_COMPAT
>  struct compat_megasas_iocpacket {
>  	u16 host_no;
>  	u16 __pad1;
> @@ -2621,7 +2620,6 @@ struct compat_megasas_iocpacket {
>  } __attribute__ ((packed));
>  
>  #define MEGASAS_IOC_FIRMWARE32	_IOWR('M', 1, struct compat_megasas_iocpacket)
> -#endif
>  
>  #define MEGASAS_IOC_FIRMWARE	_IOWR('M', 1, struct megasas_iocpacket)
>  #define MEGASAS_IOC_GET_AEN	_IOW('M', 3, struct megasas_aen)
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index c3de69f3bee8..d91951ee16ab 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -8279,16 +8279,18 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
>  	 * copy out the sense
>  	 */
>  	if (ioc->sense_len) {
> +		void __user *uptr;
>  		/*
>  		 * 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));

should the u32 * here by a compat_uptr *? Not tat it would make a
difference, just better document what we are doing.

> +	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;
> +		}

I don't think we need the braces here.

> +	return ioc;
> +out:
> +	kfree(ioc);
> +
> +	return ERR_PTR(err);

spurious empty line.

> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -91,6 +91,11 @@
>  	static inline long __do_compat_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
>  #endif /* COMPAT_SYSCALL_DEFINEx */
>  
> +struct compat_iovec {
> +	compat_uptr_t	iov_base;
> +	compat_size_t	iov_len;
> +};
> +
>  #ifdef CONFIG_COMPAT
>  
>  #ifndef compat_user_stack_pointer
> @@ -248,11 +253,6 @@ typedef struct compat_siginfo {
>  	} _sifields;
>  } compat_siginfo_t;
>  
> -struct compat_iovec {
> -	compat_uptr_t	iov_base;
> -	compat_size_t	iov_len;
> -};

This should probably go into a separate patch instead of being hidden
in a driver patch.

But except for these nitpicks the change looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>