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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3287 bytes --]

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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;				\
 })							
 
-#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))




[-- Attachment #2: x86-get-put-user.patch --]
[-- Type: text/plain, Size: 3326 bytes --]

x86: polish __{get,put}_user_{,no}check()

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

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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;				\
 })							
 
-#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))

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

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

[Andrew Cooper]

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@suse.com>
> ---
> 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@citrix.com>

>  
> -#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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 02.05.17 at 16:28, <andrew.cooper3@citrix.com> wrote:
> 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.

Well, if you're convinced this is better. I did consider doing so, but
didn't because it's a relatively small portion of code which does use
spaces at present.

>> --- 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() ?

Oh, sure. I didn't notice there was a similar issue here.

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

Thanks, but please let me know whether you feel strongly about
using spaces instead of tabs.

Jan


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

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

[Andrew Cooper]

On 02/05/17 15:40, Jan Beulich wrote:
>>>> On 02.05.17 at 16:28, <andrew.cooper3@citrix.com> wrote:
>> 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.
> Well, if you're convinced this is better. I did consider doing so, but
> didn't because it's a relatively small portion of code which does use
> spaces at present.
>
>>> --- 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() ?
> Oh, sure. I didn't notice there was a similar issue here.
>
>> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks, but please let me know whether you feel strongly about
> using spaces instead of tabs.

I'd prefer spaces (for overall consistency in the file), but my R-by
isn't conditional on it (as the file is already very mixed).

~Andrew

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

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

[Julien Grall]

Hi,

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
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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.

Andrew, do you have any opinion to get this patch in Xen 4.9?

Cheers,

-- 
Julien Grall

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

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

[Andrew Cooper]

On 04/05/17 09:52, Julien Grall wrote:
> Hi,
>
> 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
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> 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.
>
> Andrew, do you have any opinion to get this patch in Xen 4.9?

It should go in.

~Andrew

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

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

[Julien Grall]

Hi Andrew,

On 04/05/17 18:52, Andrew Cooper wrote:
> On 04/05/17 09:52, Julien Grall wrote:
>> Hi,
>>
>> 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
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> 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.
>>
>> Andrew, do you have any opinion to get this patch in Xen 4.9?
>
> It should go in.

Release-acked-by: Julien grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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