| Message ID | 20250209105600.3388-2-david.laight.linux@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | uaccess: Add masked_user_read_access_begin | expand |
On Sun, 9 Feb 2025 at 02:56, David Laight <david.laight.linux@gmail.com> wrote: > > Code can then be changed: > - if (!user_read_access_begin(from, sizeof(*from))) > + if (!masked_user_read_access_begin(&from, sizeof(*from))) > return -EFAULT; I really dislike the use of "pass pointer to simple variable you are going to change" interfaces which is why I didn't do it this way. It's actually one of my least favorite parts of C - and one of the things that Rust got right - because the whole "error separate from return value" is such an important thing for graceful error handling. And it's also why we use error pointers in the kernel: because I really *hated* all the cases where people were returning separate errors and results. The kernel error pointer thing may seem obvious and natural to people now, but it was a fairly big change at the time. I'd actually much prefer the "goto efault" model that "unsafe_get/put_user()" uses than passing in the other return value with a pointer. I wish we had a good error handling model. Not the async crap that is exceptions with try/catch (or setjmp/longjmp - the horror). Just nice synchronous error handling that doesn't require the whole "hide return value as a in-out argument". I know people think 'goto' and labels are bad. But I seriously think they are better and more legible constructs than the 'return value in arguments'. Yes, you can make spaghetti code with goto and labels. But 'return value in arguments' is worse, because it makes the *data flow* harder to see. Linus
On Sun, 9 Feb 2025 09:40:05 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, 9 Feb 2025 at 02:56, David Laight <david.laight.linux@gmail.com> wrote: > > > > Code can then be changed: > > - if (!user_read_access_begin(from, sizeof(*from))) > > + if (!masked_user_read_access_begin(&from, sizeof(*from))) > > return -EFAULT; > > I really dislike the use of "pass pointer to simple variable you are > going to change" interfaces which is why I didn't do it this way. For real functions they do generate horrid code. But since this is a define the *&from is optimised away. Definitely better than just passing 'from' and having it unexpectedly changed (why did C++ allow that one?). > It's actually one of my least favorite parts of C - and one of the > things that Rust got right - because the whole "error separate from > return value" is such an important thing for graceful error handling. Especially since the ABI almost always let a register pair be returned. Shame it can't be used for anything other than double-length integers. > And it's also why we use error pointers in the kernel: because I > really *hated* all the cases where people were returning separate > errors and results. The kernel error pointer thing may seem obvious > and natural to people now, but it was a fairly big change at the time. I've a lurking plan to change getsockopt() to return error/length and move all the user copies into the syscall wrapper. Made more complicated by code that wants to return an error and a length. (They'll probably need packing into a single value that is negative - so that the decode is in the slow path.) > I'd actually much prefer the "goto efault" model that > "unsafe_get/put_user()" uses than passing in the other return value > with a pointer. > > I wish we had a good error handling model. > > Not the async crap that is exceptions with try/catch (or > setjmp/longjmp - the horror). Just nice synchronous error handling > that doesn't require the whole "hide return value as a in-out > argument". > > I know people think 'goto' and labels are bad. But I seriously think > they are better and more legible constructs than the 'return value in > arguments'. I've had to fix some 'day-job' code which had repeated 'if (!error)' clauses to avoid early return (never mind goto). Typically at least one path got the error handling wrong. At least explicit 'return error' or 'goto return_error' are easy to validate. > > Yes, you can make spaghetti code with goto and labels. But 'return > value in arguments' is worse, because it makes the *data flow* harder > to see. Hidden returns are a real nightmare - you can't even guess whether any locking (etc) is done. At least hidden goto are visible. Let me see if I can to a 'hidden goto' version. David > > Linus
On Sun, 9 Feb 2025 at 10:34, David Laight <david.laight.linux@gmail.com> wrote: > > For real functions they do generate horrid code. It';s not about the code generation,. Well, yes, it is that too in other circumstances, but that's not the deeper issue. The deeper issue is this: > > Yes, you can make spaghetti code with goto and labels. But 'return > > value in arguments' is worse, because it makes the *data flow* harder > > to see. This is the issue that is entirely independent of what the code generation is. This is the issue that affects humans reading the source. It's *really* easy to miss the "oh, that changes X" when the only little tiny sign of it is that "&" character inside the argument list, and then the *normal* mental model is that arguments are arguments *to* the function, not returns *from* the function. (And yes, I admit that is inconsistent: we have lots of cases where we have structures etc that get filled in or modified by functions, and are passed by reference all the time. But I think there's a big difference between a regular "value" like a user pointer, and a complex data structure). Linus
On Sun, 9 Feb 2025 at 10:40, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > It's *really* easy to miss the "oh, that changes X" when the only > little tiny sign of it is that "&" character inside the argument list, > and then the *normal* mental model is that arguments are arguments > *to* the function, not returns *from* the function. While I'm ranting, this is just another case of "C++ really got this very very wrong". C++ made pass-by-reference really easy, and removed the need for even that tiny marker in the caller. So then you really can't even visually see that "oh, that call changes its argument", and have to just know. Linus
On Sun, 9 Feb 2025 10:46:03 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, 9 Feb 2025 at 10:40, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > It's *really* easy to miss the "oh, that changes X" when the only > > little tiny sign of it is that "&" character inside the argument list, > > and then the *normal* mental model is that arguments are arguments > > *to* the function, not returns *from* the function. > > While I'm ranting, this is just another case of "C++ really got this > very very wrong". > > C++ made pass-by-reference really easy, and removed the need for even > that tiny marker in the caller. > > So then you really can't even visually see that "oh, that call changes > its argument", and have to just know. Or make a habit of appending '+ 0' to every argument :-) > > Linus
On Sun, 9 Feb 2025 09:40:05 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, 9 Feb 2025 at 02:56, David Laight <david.laight.linux@gmail.com> wrote: > > > > Code can then be changed: > > - if (!user_read_access_begin(from, sizeof(*from))) > > + if (!masked_user_read_access_begin(&from, sizeof(*from))) > > return -EFAULT; > > I really dislike the use of "pass pointer to simple variable you are > going to change" interfaces which is why I didn't do it this way. I'm not sure the 'goto' model works here. The issue is that the calling code mustn't use the unmasked address. You really want to make that as hard as possible. So the 'function' really does need to do an in-situ update. I did do a test compile without the &, it exploded but I didn't check whether it always would. IIRC there is a sparse check for 'user' pointers that would help. Even with the current functions, someone is bound to write: if (!masked_user_access_begin(uaddr)) return -EFAULT; unsafe_get_user(kaddr, uaddr, label); and it will all appear to be fine... (objtool might detect something because of the NULL pointer path.) You almost need it to be 'void masked_user_access_begin(&uaddr)'. David
On Sun, 9 Feb 2025 at 11:48, David Laight <david.laight.linux@gmail.com> wrote: > > You almost need it to be 'void masked_user_access_begin(&uaddr)'. Maybe we just need to make it a two-stage thing, with if (!user_access_ok(uaddr, size)) return -EFAULT; user_read_access_begin(&uaddr); unsafe_get_user(val1, &uaddr->one, Efault); unsafe_get_user(val2, &uaddr->two, Efault); user_read_access_end(); ... all done .. Efault: user_read_access_end(); return -EFAULT; and that would actually simplify some things: right now we have separate versions of the user address checking (for read/write/either): user_read_access_begin() and friends. We still need those three versions, but now we'd only need them for the simpler non-conditional case that doesn't have to bother about the size. And then if you have user address masking, user_access_ok() just unconditionally returns true and is a no-op, while user_read_access_begin() does the masking and actually enables user accesses. And if you *don't* have user address masking, user_read_access_begin() still enables user accesses and has the required speculation synchronization, but doesn't do any address checking, because user_access_ok() did that (and nothing else). That seems like it might be a reasonable compromise and fairly hard to get wrong (*)? Linus (*) Obviously anybody can get anything wrong, but if you forget the user_access_ok() entirely you're being wilful about it, and if you forget the user_read_access_begin() the code won't work, so it seems about as safe as it can be.
On Sun, 9 Feb 2025 12:40:32 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sun, 9 Feb 2025 at 11:48, David Laight <david.laight.linux@gmail.com> wrote: > > > > You almost need it to be 'void masked_user_access_begin(&uaddr)'. > > Maybe we just need to make it a two-stage thing, with > > if (!user_access_ok(uaddr, size)) > return -EFAULT; > user_read_access_begin(&uaddr); > unsafe_get_user(val1, &uaddr->one, Efault); > unsafe_get_user(val2, &uaddr->two, Efault); > user_read_access_end(); > ... all done .. > > Efault: > user_read_access_end(); > return -EFAULT; > > and that would actually simplify some things: right now we have > separate versions of the user address checking (for > read/write/either): user_read_access_begin() and friends. > > We still need those three versions, but now we'd only need them for > the simpler non-conditional case that doesn't have to bother about the > size. Except for the ppc? case which needs the size to open a bounded window. (I'm not sure how that handler r/w access.) So you either have to pass the size twice or come back to: if (!user_read_access_begin(&uaddr, size)) return -EFAULT; unsafe_get_user(...); David
On Sun, 9 Feb 2025 at 13:18, David Laight <david.laight.linux@gmail.com> wrote: > > Except for the ppc? case which needs the size to open a bounded window. It passes the size down, but I didn't actually see it *use* the size anywhere outside of the actual range check. So it has code like static __always_inline void allow_user_access(void __user *to, const void __user *from, u32 size, unsigned long dir) { BUILD_BUG_ON(!__builtin_constant_p(dir)); if (!(dir & KUAP_WRITE)) return; current->thread.kuap = (__force u32)to; uaccess_begin_32s((__force u32)to); } but notice how the size is basically not an issue. Same for the 8xx case: static __always_inline void allow_user_access(void __user *to, const void __user *from, unsigned long size, unsigned long dir) { uaccess_begin_8xx(MD_APG_INIT); } or the booke case: static __always_inline void allow_user_access(void __user *to, const void __user *from, unsigned long size, unsigned long dir) { uaccess_begin_booke(current->thread.pid); } but admittedly this is all a maze of small helper functions calling each other, so I might have missed some path. Linus
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index e9c702c1908d..5a55152c0010 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -33,6 +33,15 @@ }) #endif +/* + * Architectures can reduce the cost of validating user addresses by + * defining masked_user_access_begin(). + * It should convert any kernel address to the base of an unmapped + * page (eg that of a guard page between user and kernel addresses) + * and enable accesses to user memory. + * To avoid speculative accesses it should use ALU instructions + * (eg a compare and conditional move). + */ #ifdef masked_user_access_begin #define can_do_masked_user_access() 1 #else @@ -41,6 +50,18 @@ #define mask_user_address(src) (src) #endif +#ifdef masked_user_access_begin +#define masked_user_read_access_begin(from, size) \ + ((*(from) = masked_user_access_begin(*(from))), 1) +#define masked_user_write_access_begin(from, size) \ + ((*(from) = masked_user_access_begin(*(from))), 1) +#else +#define masked_user_read_access_begin(from, size) \ + user_read_access_begin(*(from), size) +#define masked_user_write_access_begin(from, size) \ + user_write_access_begin(*(from), size) +#endif + /* * Architectures should provide two primitives (raw_copy_{to,from}_user()) * and get rid of their private instances of copy_{to,from}_user() and
Commit 2865baf54077a added 'user address masking' to avoid the serialising instructions associated with access_ok() when using unsafe_get_user(). However the code pattern required is non-trivial. Add a new wrapper masked_user_read_access_begin() to simplify things. Code can then be changed: - if (!user_read_access_begin(from, sizeof(*from))) + if (!masked_user_read_access_begin(&from, sizeof(*from))) return -EFAULT; unsafe_get_user(xxx, &from->xxx, Efault); If address masking is supported the 'return -EFAULT' will never happen. Add the matching masked_user_write_access_begin(). Although speculative accesses aren't an issue, it saves the conditional branch. Signed-off-by: David Laight <david.laight.linux@gmail.com> --- include/linux/uaccess.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)