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@xxxxxxxx>
> ---
> 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@xxxxxx>
