| Message ID | 5908A48B0200007800155CF7@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)) > > >
>>> 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
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
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,
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
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,
--- 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))