Re: [GIT PULL] objtool/urgent for v6.5-rc2

[Vegard Nossum]

On 7/16/23 22:39, Linus Torvalds wrote:
> On Sun, 16 Jul 2023 at 11:42, Borislav Petkov <bp@xxxxxxxxx> wrote:
> > - Mark copy_iovec_from_user() __noclone in order to  prevent gcc from
> >    doing an inter-procedural optimization and confuse objtool
>
> This is painful.
>
> Isn't there some way to mark the user_access_begin() itself so that
> the compiler doesn't move it?
>
> I've pulled this thing, but it does seem like we're chasing the
> symptoms, not the deeper cause..

The deeper cause is that the [code generated by the] user_access_begin()
and user_write_access_end()/unsafe_get_user() calls end up in different
functions and objtool doesn't like that.

If you are willing to add another helper function that also takes the
label argument, you could do something like this:

#define user_access_begin_label(a,b, err_label) \
        ({ \
                asm_volatile_goto("" :::: err_label); \
                user_access_begin(a,b); \
        })

The asm_volatile_goto() isn't a real goto (it emits no instructions),
but it makes GCC believe there's a potential jump there and prevents the
compiler from splitting up the function across the label and its usage.

unsafe_get_user() already takes this same label as an argument and the
user_access_end() call is where the label is defined.

I tried first to avoid changing the uaccess API by defining a fixed
label inside user_write_access_end() and always doing "fake jumps" to
this fixed label from both user_write_access_begin() and
unsafe_get_user(). However, you run into trouble with functions like
sys_waitid() in kernel/exit.c that have multiple user_write_access_end()
calls.

You might think that you could switch them around -- define the label in
_begin() and fake-jump to it from _end(), but that doesn't work either
since _begin() needs to be an expression and labels defined inside a
statement expression are not accessible outside that expression...

Anyway, attached a proof of concept patch that fixes the objtool
warning, but I didn't even do a full build test.


Vegarddiff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 1cc756eafa44..129bf823e405 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -554,6 +554,11 @@ static __must_check __always_inline bool user_access_begin(const void __user *pt
 	return 1;
 }
 #define user_access_begin(a,b)	user_access_begin(a,b)
+#define user_access_begin_label(a,b, err_label)	\
+	({ \
+		asm_volatile_goto("" :::: err_label); \
+		user_access_begin(a,b); \
+	})
 #define user_access_end()	__uaccess_end()
 
 #define user_access_save()	smap_save()
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 3e6c9bcfa612..3309bad69eca 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -1709,7 +1709,7 @@ static int copy_compat_iovec_from_user(struct iovec *iov,
 		(const struct compat_iovec __user *)uvec;
 	int ret = -EFAULT, i;
 
-	if (!user_access_begin(uiov, nr_segs * sizeof(*uiov)))
+	if (!user_access_begin_label(uiov, nr_segs * sizeof(*uiov), uaccess_end))
 		return -EFAULT;
 
 	for (i = 0; i < nr_segs; i++) {