Re: [Xen-devel] [PATCH] x86: polish __{get,put}_user_{,no}check()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/05/17 14:23, Jan Beulich wrote:
> The primary purpose is correcting a latent bug in __get_user_check()
> (the macro has no active user at present): The access_ok() check should
> be before the actual access, or else any PV guest could initiate MMIO
> reads with side effects.
>
> Clean up all four macros at once:
> - all arguments evaluated exactly once
> - build the "check" flavor using the "nocheck" ones, instead of open
>   coding them
> - "int" is wide enough for error codes
> - name local variables without using underscores as prefixes
> - avoid pointless parentheses
> - add blanks after commas separating parameters or arguments
> - consistently use tabs for indentation

Could we use spaces?  This file is already half and half style, and
these bits of code are a long way removed from their Linux heritage.

>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> This corrects the code which would have resulted in an XSA on Xen 4.2
> and older, if those were still security supported. For that reason I at
> least want to explore whether this is a change we want to take for 4.9.
>
> --- a/xen/include/asm-x86/uaccess.h
> +++ b/xen/include/asm-x86/uaccess.h
> @@ -104,37 +104,35 @@ extern void __put_user_bad(void);
>  #define __put_user(x,ptr) \
>    __put_user_nocheck((__typeof__(*(ptr)))(x),(ptr),sizeof(*(ptr)))
>  
> -#define __put_user_nocheck(x,ptr,size)                               \
> -({                                                           \
> -     long __pu_err;                                          \
> -     __put_user_size((x),(ptr),(size),__pu_err,-EFAULT);     \
> -     __pu_err;                                               \
> +#define __put_user_nocheck(x, ptr, size)                             \
> +({                                                                   \
> +     int err_;                                                       \
> +     __put_user_size(x, ptr, size, err_, -EFAULT);                   \
> +     err_;                                                           \
>  })
>  
> -#define __put_user_check(x,ptr,size)                                 \
> +#define __put_user_check(x, ptr, size)                                       
> \
>  ({                                                                   \
> -     long __pu_err = -EFAULT;                                        \
> -     __typeof__(*(ptr)) __user *__pu_addr = (ptr);                   \
> -     if (access_ok(__pu_addr,size))                                  \
> -             __put_user_size((x),__pu_addr,(size),__pu_err,-EFAULT); \
> -     __pu_err;                                                       \
> +     __typeof__(*(ptr)) __user *ptr_ = (ptr);                        \
> +     __typeof__(size) size_ = (size);                                \
> +     access_ok(ptr_, size_) ? __put_user_nocheck(x, ptr_, size_)     \
> +                            : -EFAULT;                               \
>  })                                                   

Can you clobber the trailing whitespace on this line, like you did with
__get_user_check() ?

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

>  
> -#define __get_user_nocheck(x,ptr,size)                          \
> -({                                                              \
> -     long __gu_err;                                          \
> -     __get_user_size((x),(ptr),(size),__gu_err,-EFAULT);     \
> -     __gu_err;                                               \
> +#define __get_user_nocheck(x, ptr, size)                             \
> +({                                                                   \
> +     int err_;                                                       \
> +     __get_user_size(x, ptr, size, err_, -EFAULT);                   \
> +     err_;                                                           \
>  })
>  
> -#define __get_user_check(x,ptr,size)                            \
> -({                                                              \
> -     long __gu_err;                                          \
> -     __typeof__(*(ptr)) __user *__gu_addr = (ptr);           \
> -     __get_user_size((x),__gu_addr,(size),__gu_err,-EFAULT); \
> -     if (!access_ok(__gu_addr,size)) __gu_err = -EFAULT;     \
> -     __gu_err;                                               \
> -})                                                   
> +#define __get_user_check(x, ptr, size)                                       
> \
> +({                                                                   \
> +     __typeof__(*(ptr)) __user *ptr_ = (ptr);                        \
> +     __typeof__(size) size_ = (size);                                \
> +     access_ok(ptr_, size_) ? __get_user_nocheck(x, ptr_, size_)     \
> +                            : -EFAULT;                               \
> +})
>  
>  struct __large_struct { unsigned long buf[100]; };
>  #define __m(x) (*(const struct __large_struct *)(x))
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel