[PATCH] um: fix stub location calculation

[PATCH] um: fix stub location calculation

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

In commit 9f0b4807a44f ("um: rework userspace stubs to not hard-code
stub location") I changed stub_segv_handler() to do a calculation with
a pointer to a stack variable to find the data page that we're using
for the stack and the rest of the data. This same commit was meant to
do it as well for stub_clone_handler(), but the change inadvertently
went into commit 84b2789d6115 ("um: separate child and parent errors
in clone stub") instead.

This was reported to not be compiled correctly by gcc 5, causing the
code to crash here. I'm not sure why, perhaps it's UB because the var
isn't initialized? In any case, this trick always seemed bad, so just
create a new inline function that does the calculation in assembly.

Reported-by: subashab@codeaurora.org
Fixes: 9f0b4807a44f ("um: rework userspace stubs to not hard-code stub location")
Fixes: 84b2789d6115 ("um: separate child and parent errors in clone stub")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/um/kernel/skas/clone.c         |  3 +--
 arch/x86/um/shared/sysdep/stub_32.h | 12 ++++++++++++
 arch/x86/um/shared/sysdep/stub_64.h | 12 ++++++++++++
 arch/x86/um/stub_segv.c             |  3 +--
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c
index 592cdb138441..c0d2d592b31b 100644
--- a/arch/um/kernel/skas/clone.c
+++ b/arch/um/kernel/skas/clone.c
@@ -24,8 +24,7 @@
 void __attribute__ ((__section__ (".__syscall_stub")))
 stub_clone_handler(void)
 {
-	int stack;
-	struct stub_data *data = (void *) ((unsigned long)&stack & ~(UM_KERN_PAGE_SIZE - 1));
+	struct stub_data *data = get_stub_page();
 	long err;
 
 	err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | SIGCHLD,
diff --git a/arch/x86/um/shared/sysdep/stub_32.h b/arch/x86/um/shared/sysdep/stub_32.h
index b95db9daf0e8..4c6c2be0c899 100644
--- a/arch/x86/um/shared/sysdep/stub_32.h
+++ b/arch/x86/um/shared/sysdep/stub_32.h
@@ -101,4 +101,16 @@ static inline void remap_stack_and_trap(void)
 		"memory");
 }
 
+static __always_inline void *get_stub_page(void)
+{
+	unsigned long ret;
+
+	asm volatile (
+		"movl %%esp,%0 ;"
+		"andl %1,%0"
+		: "=a" (ret)
+		: "g" (~(UM_KERN_PAGE_SIZE - 1)));
+
+	return (void *)ret;
+}
 #endif
diff --git a/arch/x86/um/shared/sysdep/stub_64.h b/arch/x86/um/shared/sysdep/stub_64.h
index 6e2626b77a2e..e9c4b2b38803 100644
--- a/arch/x86/um/shared/sysdep/stub_64.h
+++ b/arch/x86/um/shared/sysdep/stub_64.h
@@ -108,4 +108,16 @@ static inline void remap_stack_and_trap(void)
 		__syscall_clobber, "r10", "r8", "r9");
 }
 
+static __always_inline void *get_stub_page(void)
+{
+	unsigned long ret;
+
+	asm volatile (
+		"movq %%rsp,%0 ;"
+		"andq %1,%0"
+		: "=a" (ret)
+		: "g" (~(UM_KERN_PAGE_SIZE - 1)));
+
+	return (void *)ret;
+}
 #endif
diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c
index 21836eaf1725..f7eefba034f9 100644
--- a/arch/x86/um/stub_segv.c
+++ b/arch/x86/um/stub_segv.c
@@ -11,9 +11,8 @@
 void __attribute__ ((__section__ (".__syscall_stub")))
 stub_segv_handler(int sig, siginfo_t *info, void *p)
 {
-	int stack;
+	struct faultinfo *f = get_stub_page();
 	ucontext_t *uc = p;
-	struct faultinfo *f = (void *)(((unsigned long)&stack) & ~(UM_KERN_PAGE_SIZE - 1));
 
 	GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext);
 	trap_myself();
-- 
2.31.1


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] um: fix stub location calculation

[Richard Weinberger]

On Tue, Jul 13, 2021 at 11:47 PM Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> In commit 9f0b4807a44f ("um: rework userspace stubs to not hard-code
> stub location") I changed stub_segv_handler() to do a calculation with
> a pointer to a stack variable to find the data page that we're using
> for the stack and the rest of the data. This same commit was meant to
> do it as well for stub_clone_handler(), but the change inadvertently
> went into commit 84b2789d6115 ("um: separate child and parent errors
> in clone stub") instead.
>
> This was reported to not be compiled correctly by gcc 5, causing the
> code to crash here. I'm not sure why, perhaps it's UB because the var
> isn't initialized? In any case, this trick always seemed bad, so just
> create a new inline function that does the calculation in assembly.

My best guess is that gcc 5 sees only local modifications, but no further reads.
So it treats it as dead store.

> Reported-by: subashab@codeaurora.org
> Fixes: 9f0b4807a44f ("um: rework userspace stubs to not hard-code stub location")
> Fixes: 84b2789d6115 ("um: separate child and parent errors in clone stub")
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

BTW: Marking data/f as volatile fixes the problem too.
That way gcc no longer optimizes data/f away.

diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c
index 592cdb1..6331941 100644
--- a/arch/um/kernel/skas/clone.c
+++ b/arch/um/kernel/skas/clone.c
@@ -25,7 +25,7 @@ void __attribute__ ((__section__ (".__syscall_stub")))
 stub_clone_handler(void)
 {
        int stack;
-       struct stub_data *data = (void *) ((unsigned long)&stack &
~(UM_KERN_PAGE_SIZE - 1));
+       volatile struct stub_data *data = (void *) ((unsigned
long)&stack & ~(UM_KERN_PAGE_SIZE - 1));
        long err;

        err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | SIGCHLD,
diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c
index 21836ea..87c3aef 100644
--- a/arch/x86/um/stub_segv.c
+++ b/arch/x86/um/stub_segv.c
@@ -13,7 +13,7 @@ stub_segv_handler(int sig, siginfo_t *info, void *p)
 {
        int stack;
        ucontext_t *uc = p;
-       struct faultinfo *f = (void *)(((unsigned long)&stack) &
~(UM_KERN_PAGE_SIZE - 1));
+       volatile struct faultinfo *f = (void *)(((unsigned
long)&stack) & ~(UM_KERN_PAGE_SIZE - 1));

        GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext);
        trap_myself();

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] um: fix stub location calculation

[subashab]

On 2021-07-13 16:11, Richard Weinberger wrote:
> On Tue, Jul 13, 2021 at 11:47 PM Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> 
>> From: Johannes Berg <johannes.berg@intel.com>
>> 
>> In commit 9f0b4807a44f ("um: rework userspace stubs to not hard-code
>> stub location") I changed stub_segv_handler() to do a calculation with
>> a pointer to a stack variable to find the data page that we're using
>> for the stack and the rest of the data. This same commit was meant to
>> do it as well for stub_clone_handler(), but the change inadvertently
>> went into commit 84b2789d6115 ("um: separate child and parent errors
>> in clone stub") instead.
>> 
>> This was reported to not be compiled correctly by gcc 5, causing the
>> code to crash here. I'm not sure why, perhaps it's UB because the var
>> isn't initialized? In any case, this trick always seemed bad, so just
>> create a new inline function that does the calculation in assembly.
> 
> My best guess is that gcc 5 sees only local modifications, but no 
> further reads.
> So it treats it as dead store.
> 
>> Reported-by: subashab@codeaurora.org
>> Fixes: 9f0b4807a44f ("um: rework userspace stubs to not hard-code stub 
>> location")
>> Fixes: 84b2789d6115 ("um: separate child and parent errors in clone 
>> stub")
>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> BTW: Marking data/f as volatile fixes the problem too.
> That way gcc no longer optimizes data/f away.
> 
> diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c
> index 592cdb1..6331941 100644
> --- a/arch/um/kernel/skas/clone.c
> +++ b/arch/um/kernel/skas/clone.c
> @@ -25,7 +25,7 @@ void __attribute__ ((__section__ 
> (".__syscall_stub")))
>  stub_clone_handler(void)
>  {
>         int stack;
> -       struct stub_data *data = (void *) ((unsigned long)&stack &
> ~(UM_KERN_PAGE_SIZE - 1));
> +       volatile struct stub_data *data = (void *) ((unsigned
> long)&stack & ~(UM_KERN_PAGE_SIZE - 1));
>         long err;
> 
>         err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | 
> SIGCHLD,
> diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c
> index 21836ea..87c3aef 100644
> --- a/arch/x86/um/stub_segv.c
> +++ b/arch/x86/um/stub_segv.c
> @@ -13,7 +13,7 @@ stub_segv_handler(int sig, siginfo_t *info, void *p)
>  {
>         int stack;
>         ucontext_t *uc = p;
> -       struct faultinfo *f = (void *)(((unsigned long)&stack) &
> ~(UM_KERN_PAGE_SIZE - 1));
> +       volatile struct faultinfo *f = (void *)(((unsigned
> long)&stack) & ~(UM_KERN_PAGE_SIZE - 1));
> 
>         GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext);
>         trap_myself();

Both of these work for me. Thanks for the help.

Reported-and-tested-by: Subash Abhinov Kasiviswanathan 
<subashab@codeaurora.org>

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] um: fix stub location calculation

[YiFei Zhu]

On Tue, Jul 13, 2021 at 5:11 PM Richard Weinberger
<richard.weinberger@gmail.com> wrote:
>
> On Tue, Jul 13, 2021 at 11:47 PM Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > In commit 9f0b4807a44f ("um: rework userspace stubs to not hard-code
> > stub location") I changed stub_segv_handler() to do a calculation with
> > a pointer to a stack variable to find the data page that we're using
> > for the stack and the rest of the data. This same commit was meant to
> > do it as well for stub_clone_handler(), but the change inadvertently
> > went into commit 84b2789d6115 ("um: separate child and parent errors
> > in clone stub") instead.
> >
> > This was reported to not be compiled correctly by gcc 5, causing the
> > code to crash here. I'm not sure why, perhaps it's UB because the var
> > isn't initialized? In any case, this trick always seemed bad, so just
> > create a new inline function that does the calculation in assembly.
>
> My best guess is that gcc 5 sees only local modifications, but no further reads.
> So it treats it as dead store.
>
> > Reported-by: subashab@codeaurora.org
> > Fixes: 9f0b4807a44f ("um: rework userspace stubs to not hard-code stub location")
> > Fixes: 84b2789d6115 ("um: separate child and parent errors in clone stub")
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>
> BTW: Marking data/f as volatile fixes the problem too.
> That way gcc no longer optimizes data/f away.

Yeah, in the bad compilation,

(gdb) disas stub_segv_handler
Dump of assembler code for function stub_segv_handler:
   0x000000006033d0c5 <+0>:    push   %rbp
   0x000000006033d0c6 <+1>:    mov    %rsp,%rbp
   0x000000006033d0c9 <+4>:    int3
   0x000000006033d0ca <+5>:    pop    %rbp
   0x000000006033d0cb <+6>:    retq
End of assembler dump.

The store is optimized away -> the faultinfo is unmodified -> the segv
handler treats the fault as unfixable -> init dead.

>
> diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c
> index 592cdb1..6331941 100644
> --- a/arch/um/kernel/skas/clone.c
> +++ b/arch/um/kernel/skas/clone.c
> @@ -25,7 +25,7 @@ void __attribute__ ((__section__ (".__syscall_stub")))
>  stub_clone_handler(void)
>  {
>         int stack;
> -       struct stub_data *data = (void *) ((unsigned long)&stack &
> ~(UM_KERN_PAGE_SIZE - 1));
> +       volatile struct stub_data *data = (void *) ((unsigned
> long)&stack & ~(UM_KERN_PAGE_SIZE - 1));
>         long err;
>
>         err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | SIGCHLD,
> diff --git a/arch/x86/um/stub_segv.c b/arch/x86/um/stub_segv.c
> index 21836ea..87c3aef 100644
> --- a/arch/x86/um/stub_segv.c
> +++ b/arch/x86/um/stub_segv.c
> @@ -13,7 +13,7 @@ stub_segv_handler(int sig, siginfo_t *info, void *p)
>  {
>         int stack;
>         ucontext_t *uc = p;
> -       struct faultinfo *f = (void *)(((unsigned long)&stack) &
> ~(UM_KERN_PAGE_SIZE - 1));
> +       volatile struct faultinfo *f = (void *)(((unsigned
> long)&stack) & ~(UM_KERN_PAGE_SIZE - 1));
>
>         GET_FAULTINFO_FROM_MC(*f, &uc->uc_mcontext);
>         trap_myself();
>
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH] um: fix stub location calculation

[Johannes Berg]

On Tue, 2021-07-13 at 17:44 -0500, YiFei Zhu wrote:
> 
> The store is optimized away -> the faultinfo is unmodified -> the segv
> handler treats the fault as unfixable -> init dead.

Yeah. I was confused about this in the clone case, but even there of
course the store is important because that's the child's PID in the
parent ... I somehow looked at the code last night and thought it was
only a broken error case.

I'll leave it to Richard to figure out which of the two things to apply,
I guess. And it should probably come with a Cc: stable, which I forgot
on my patch.

johannes


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um