From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Sean Christopherson <sean.j.christopherson@intel.com>,
Linus Torvalds <torvalds@linux-foundation.org>
Cc: "Daniel Díaz" <daniel.diaz@linaro.org>,
"Naresh Kamboju" <naresh.kamboju@linaro.org>,
"Stephen Rothwell" <sfr@canb.auug.org.au>,
"Matthew Wilcox (Oracle)" <willy@infradead.org>,
zenglg.jy@cn.fujitsu.com,
"Peter Zijlstra (Intel)" <peterz@infradead.org>,
"Viresh Kumar" <viresh.kumar@linaro.org>,
"X86 ML" <x86@kernel.org>,
"open list" <linux-kernel@vger.kernel.org>,
lkft-triage@lists.linaro.org,
"Eric W. Biederman" <ebiederm@xmission.com>,
linux-mm <linux-mm@kvack.org>,
linux-m68k <linux-m68k@lists.linux-m68k.org>,
"Linux-Next Mailing List" <linux-next@vger.kernel.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
kasan-dev <kasan-dev@googlegroups.com>,
"Dmitry Vyukov" <dvyukov@google.com>,
"Geert Uytterhoeven" <geert@linux-m68k.org>,
"Christian Brauner" <christian.brauner@ubuntu.com>,
"Ingo Molnar" <mingo@redhat.com>, "LTP List" <ltp@lists.linux.it>,
"Al Viro" <viro@zeniv.linux.org.uk>
Subject: Re: [LTP] mmstress[1309]: segfault at 7f3d71a36ee8 ip 00007f3d77132bdf sp 00007f3d71a36ee8 error 4 in libc-2.27.so[7f3d77058000+1aa000]
Date: Fri, 23 Oct 2020 09:14:21 +0200 [thread overview]
Message-ID: <356811ab-cb08-7685-ca01-fe58b5654953@rasmusvillemoes.dk> (raw)
In-Reply-To: <20201023050214.GG23681@linux.intel.com>
On 23/10/2020 07.02, Sean Christopherson wrote:
> On Thu, Oct 22, 2020 at 08:05:05PM -0700, Linus Torvalds wrote:
>> On Thu, Oct 22, 2020 at 6:36 PM Daniel Díaz <daniel.diaz@linaro.org> wrote:
>>>
>>> The kernel Naresh originally referred to is here:
>>> https://builds.tuxbuild.com/SCI7Xyjb7V2NbfQ2lbKBZw/
>>
>> Thanks.
>>
>> And when I started looking at it, I realized that my original idea
>> ("just look for __put_user_nocheck_X calls, there aren't so many of
>> those") was garbage, and that I was just being stupid.
>>
>> Yes, the commit that broke was about __put_user(), but in order to not
>> duplicate all the code, it re-used the regular put_user()
>> infrastructure, and so all the normal put_user() calls are potential
>> problem spots too if this is about the compiler interaction with KASAN
>> and the asm changes.
>>
>> So it's not just a couple of special cases to look at, it's all the
>> normal cases too.
>>
>> Ok, back to the drawing board, but I think reverting it is probably
>> the right thing to do if I can't think of something smart.
>>
>> That said, since you see this on x86-64, where the whole ugly trick with that
>>
>> register asm("%"_ASM_AX)
>>
>> is unnecessary (because the 8-byte case is still just a single
>> register, no %eax:%edx games needed), it would be interesting to hear
>> if the attached patch fixes it. That would confirm that the problem
>> really is due to some register allocation issue interaction (or,
>> alternatively, it would tell me that there's something else going on).
>
> I haven't reproduced the crash, but I did find a smoking gun that confirms the
> "register shenanigans are evil shenanigans" theory. I ran into a similar thing
> recently where a seemingly innocuous line of code after loading a value into a
> register variable wreaked havoc because it clobbered the input register.
>
> This put_user() in schedule_tail():
>
> if (current->set_child_tid)
> put_user(task_pid_vnr(current), current->set_child_tid);
>
> generates the following assembly with KASAN out-of-line:
>
> 0xffffffff810dccc9 <+73>: xor %edx,%edx
> 0xffffffff810dcccb <+75>: xor %esi,%esi
> 0xffffffff810dcccd <+77>: mov %rbp,%rdi
> 0xffffffff810dccd0 <+80>: callq 0xffffffff810bf5e0 <__task_pid_nr_ns>
> 0xffffffff810dccd5 <+85>: mov %r12,%rdi
> 0xffffffff810dccd8 <+88>: callq 0xffffffff81388c60 <__asan_load8>
> 0xffffffff810dccdd <+93>: mov 0x590(%rbp),%rcx
> 0xffffffff810dcce4 <+100>: callq 0xffffffff817708a0 <__put_user_4>
> 0xffffffff810dcce9 <+105>: pop %rbx
> 0xffffffff810dccea <+106>: pop %rbp
> 0xffffffff810dcceb <+107>: pop %r12
>
> __task_pid_nr_ns() returns the pid in %rax, which gets clobbered by
> __asan_load8()'s check on current for the current->set_child_tid dereference.
>
Yup, and you don't need KASAN to implicitly generate function calls for
you. With x86_64 defconfig, I get
extern u64 __user *get_destination(int x, int y);
void pu_test(void)
{
u64 big = 0x1234abcd5678;
if (put_user(big, get_destination(4, 5)))
pr_warn("uh");
}
to generate
0000000000004d60 <pu_test>:
4d60: 53 push %rbx
4d61: be 05 00 00 00 mov $0x5,%esi
4d66: bf 04 00 00 00 mov $0x4,%edi
4d6b: e8 00 00 00 00 callq 4d70 <pu_test+0x10>
4d6c: R_X86_64_PC32 get_destination-0x4
4d70: 48 89 c1 mov %rax,%rcx
4d73: e8 00 00 00 00 callq 4d78 <pu_test+0x18>
4d74: R_X86_64_PC32 __put_user_8-0x4
4d78: 85 c9 test %ecx,%ecx
4d7a: 75 02 jne 4d7e <pu_test+0x1e>
4d7c: 5b pop %rbx
4d7d: c3 retq
4d7e: 5b pop %rbx
4d7f: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
4d82: R_X86_64_32S .rodata.str1.1+0xfa
4d86: e9 00 00 00 00 jmpq 4d8b <pu_test+0x2b>
4d87: R_X86_64_PC32 printk-0x4
That's certainly garbage. Now, I don't know if it's a sufficient fix (or
could break something else), but the obvious first step of rearranging
so that the ptr argument is evaluated before the assignment to __val_pu
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 477c503f2753..b5d3290fcd09 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -235,13 +235,13 @@ extern void __put_user_nocheck_8(void);
#define do_put_user_call(fn,x,ptr) \
({ \
int __ret_pu; \
- register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX); \
+ __typeof__(ptr) __ptr = (ptr); \
+ register __typeof__(*(ptr)) __val_pu asm("%"_ASM_AX) = (x); \
__chk_user_ptr(ptr); \
- __val_pu = (x); \
asm volatile("call __" #fn "_%P[size]" \
: "=c" (__ret_pu), \
ASM_CALL_CONSTRAINT \
- : "0" (ptr), \
+ : "0" (__ptr), \
"r" (__val_pu), \
[size] "i" (sizeof(*(ptr))) \
:"ebx"); \
at least gets us
0000000000004d60 <pu_test>:
4d60: 53 push %rbx
4d61: be 05 00 00 00 mov $0x5,%esi
4d66: bf 04 00 00 00 mov $0x4,%edi
4d6b: e8 00 00 00 00 callq 4d70 <pu_test+0x10>
4d6c: R_X86_64_PC32 get_destination-0x4
4d70: 48 89 c1 mov %rax,%rcx
4d73: 48 b8 78 56 cd ab 34 movabs $0x1234abcd5678,%rax
4d7a: 12 00 00
4d7d: e8 00 00 00 00 callq 4d82 <pu_test+0x22>
4d7e: R_X86_64_PC32 __put_user_8-0x4
FWIW, https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html
does warn about function calls and other things that might clobber the
register variables between the assignment and the use as an input
(though the case of evaluating other input operands is not explicitly
mentioned).
Rasmus
next prev parent reply other threads:[~2020-10-23 7:14 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-21 16:58 mmstress[1309]: segfault at 7f3d71a36ee8 ip 00007f3d77132bdf sp 00007f3d71a36ee8 error 4 in libc-2.27.so[7f3d77058000+1aa000] Naresh Kamboju
2020-10-21 17:05 ` Linus Torvalds
2020-10-21 17:22 ` Naresh Kamboju
2020-10-22 20:55 ` Naresh Kamboju
2020-10-22 23:43 ` Linus Torvalds
2020-10-23 0:11 ` Linus Torvalds
2020-10-23 0:22 ` Linus Torvalds
2020-10-23 1:36 ` [LTP] " Daniel Díaz
2020-10-23 3:05 ` Linus Torvalds
2020-10-23 5:02 ` Sean Christopherson
2020-10-23 7:14 ` Rasmus Villemoes [this message]
2020-10-23 15:54 ` Linus Torvalds
2020-10-23 16:32 ` Linus Torvalds
2020-10-23 17:50 ` Naresh Kamboju
2020-10-23 15:52 ` Linus Torvalds
2020-10-23 17:00 ` Naresh Kamboju
2020-10-23 17:50 ` Linus Torvalds
2020-10-23 21:15 ` Song Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=356811ab-cb08-7685-ca01-fe58b5654953@rasmusvillemoes.dk \
--to=linux@rasmusvillemoes.dk \
--cc=christian.brauner@ubuntu.com \
--cc=daniel.diaz@linaro.org \
--cc=dvyukov@google.com \
--cc=ebiederm@xmission.com \
--cc=geert@linux-m68k.org \
--cc=kasan-dev@googlegroups.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-m68k@lists.linux-m68k.org \
--cc=linux-mm@kvack.org \
--cc=linux-next@vger.kernel.org \
--cc=lkft-triage@lists.linaro.org \
--cc=ltp@lists.linux.it \
--cc=mingo@redhat.com \
--cc=naresh.kamboju@linaro.org \
--cc=peterz@infradead.org \
--cc=sean.j.christopherson@intel.com \
--cc=sfr@canb.auug.org.au \
--cc=tglx@linutronix.de \
--cc=torvalds@linux-foundation.org \
--cc=viresh.kumar@linaro.org \
--cc=viro@zeniv.linux.org.uk \
--cc=willy@infradead.org \
--cc=x86@kernel.org \
--cc=zenglg.jy@cn.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).