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

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

[Borislav Petkov]

Hi Linus,

please pull two urgent objtool fixes for 6.5.

Thx.

---

The following changes since commit 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5:

  Linux 6.5-rc1 (2023-07-09 13:53:13 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/objtool_urgent_for_v6.5_rc2

for you to fetch changes up to 719a937b7003933de1298ffa4b881dd6a234e244:

  iov_iter: Mark copy_iovec_from_user() noclone (2023-07-10 09:52:28 +0200)

----------------------------------------------------------------
- Mark copy_iovec_from_user() __noclone in order to  prevent gcc from
  doing an inter-procedural optimization and confuse objtool

- Initialize struct elf fully to avoid build failures

----------------------------------------------------------------
Michal Kubecek (1):
      objtool: initialize all of struct elf

Peter Zijlstra (1):
      iov_iter: Mark copy_iovec_from_user() noclone

 lib/iov_iter.c      | 2 +-
 tools/objtool/elf.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Linus Torvalds]

On Sun, 16 Jul 2023 at 11:42, Borislav Petkov <bp@alien8.de> 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..

            Linus

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

[pr-tracker-bot]

The pull request you sent on Sun, 16 Jul 2023 20:42:04 +0200:

> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/objtool_urgent_for_v6.5_rc2

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8a3e4a64849eb9da0e8c7e693978499562581631

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[Vegard Nossum]

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


On 7/16/23 22:39, Linus Torvalds wrote:
> On Sun, 16 Jul 2023 at 11:42, Borislav Petkov <bp@alien8.de> 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.


Vegard

[-- Attachment #2: uaccess.patch --]
[-- Type: text/x-patch, Size: 1049 bytes --]

diff --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++) {

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

[Linus Torvalds]

On Tue, 18 Jul 2023 at 04:02, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>
> If you are willing to add another helper function that also takes the
> label argument, you could do something like this:

Ugh. That's a bit uglier than I was hoping for.

I was hoping more like "use this special attribute to mark this asm as
being something that cannot be moved across function boundaries".

Now a "we have to make up horrible games and pass in labels that are
not used and use a completely different API".

Oh well.

I guess in the end we can just continue to work around this.

After all, in many ways this isn't about the code - the AC setting and
clearing is fine - it's about objtool not doing the analysis across
function boundaries.

I guess we could have just marked these iovec copy functions
"always_inline" too, and avoided it that way.

              Linus