[PATCH 0/2] uaccess: Add masked_user_read_access_begin

[PATCH 0/2] uaccess: Add masked_user_read_access_begin

[David Laight]

The code pattern for masked user access is unwieldy.
Add a new wrapper masked_user_read_access_begin() to simplify it.
Add the equivalent write wrapper.

Change fs/select.c to use the new wrappers when reading the
sigset_argpack and when writing out the result of poll.

The futex code could also be changed.

Note that this might conflict with the patch to change
get_sigset_argpack to __always_inline.

David Laight (2):
  uaccess: Simplify code pattern for masked user copies
  fs: Use masked_user_read_access_begin()

 fs/select.c             |  8 +++-----
 include/linux/uaccess.h | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+), 5 deletions(-)

-- 
2.39.5

[PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[David Laight]

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

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
-- 
2.39.5

[PATCH 2/2] fs: Use masked_user_read_access_begin()

[David Laight]

Commit 2865baf54077a changed get_sigset_argpack() to use
masked_user_access_begin() and avoid a slow synchronisation instruction.

Replace the rather unwieldy code pattern with
masked_user_read_access_begin().
Make the same change to get_compat_sigset_argpack().

Use masked_user_write_access_begin() in do_sys_poll() to avoid
the size calculation and conditional branch for access_ok().

Code generated for get_sigset_argpack() is identical.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 fs/select.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/select.c b/fs/select.c
index 7da531b1cf6b..abe2c1a57274 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -776,9 +776,7 @@ static inline int get_sigset_argpack(struct sigset_argpack *to,
 {
 	// the path is hot enough for overhead of copy_from_user() to matter
 	if (from) {
-		if (can_do_masked_user_access())
-			from = masked_user_access_begin(from);
-		else if (!user_read_access_begin(from, sizeof(*from)))
+		if (!masked_user_read_access_begin(&from, sizeof(*from)))
 			return -EFAULT;
 		unsafe_get_user(to->p, &from->p, Efault);
 		unsafe_get_user(to->size, &from->size, Efault);
@@ -1009,7 +1007,7 @@ static int do_sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 	fdcount = do_poll(head, &table, end_time);
 	poll_freewait(&table);
 
-	if (!user_write_access_begin(ufds, nfds * sizeof(*ufds)))
+	if (!masked_user_write_access_begin(&ufds, nfds * sizeof(*ufds)))
 		goto out_fds;
 
 	for (walk = head; walk; walk = walk->next) {
@@ -1347,7 +1345,7 @@ static inline int get_compat_sigset_argpack(struct compat_sigset_argpack *to,
 					    struct compat_sigset_argpack __user *from)
 {
 	if (from) {
-		if (!user_read_access_begin(from, sizeof(*from)))
+		if (!masked_user_read_access_begin(&from, sizeof(*from)))
 			return -EFAULT;
 		unsafe_get_user(to->p, &from->p, Efault);
 		unsafe_get_user(to->size, &from->size, Efault);
-- 
2.39.5

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[Linus Torvalds]

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

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[David Laight]

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

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[Linus Torvalds]

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

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[Linus Torvalds]

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

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[David Laight]

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

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[David Laight]

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

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[Linus Torvalds]

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.

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[David Laight]

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

Re: [PATCH 1/2] uaccess: Simplify code pattern for masked user copies

[Linus Torvalds]

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