Re: [PATCH] powerpc: Fix sigset_t copy

2021-11-10 Thread Finn Thain
On Wed, 10 Nov 2021, Christophe Leroy wrote:

> Le 10/11/2021 à 00:47, Finn Thain a écrit :
> 
> > Christophe, I hope this change is the one you wanted to see upstream 
> > (?). If it is acceptable please add your signed-off-by tag.
> 
> I'm on holidays, I was planing to handle this next week.
> 

OK. I'll leave it with you.

Re: [PATCH] powerpc: Fix sigset_t copy

2021-11-10 Thread Christophe Leroy




Le 10/11/2021 à 00:47, Finn Thain a écrit :

From: Christophe Leroy 

The conversion from __copy_from_user() to __get_user() introduced a
regression in __get_user_sigset() in v5.13. The bug was subsequently
copied and pasted in unsafe_get_user_sigset().

The regression was reported by users of the Xorg packages distributed in
Debian/powerpc --

 "The symptoms are that the fb screen goes blank, with the backlight
 remaining on and no errors logged in /var/log; wdm (or startx) run
 with no effect (I tried logging in in the blind, with no effect).
 And they are hard to kill, requiring 'kill -KILL ...'"

Fix the regression by casting the __get_user() assignment lvalue to u64
so that the entire struct gets copied.

Cc: Christophe Leroy 
Cc: Christopher M. Riedl 
Link: 
https://lore.kernel.org/linuxppc-dev/FEtBUOuFPMN4zJy4bIOqz6C4xoliCbTxS7VtMKD6UZkbvEbycUceRgGAd7e9-trRdwVN3hWAbQi0qrNx8Zgn8niTQf2KPVdw-W35czDIaeQ=@protonmail.com/
Fixes: 887f3ceb51cd ("powerpc/signal32: Convert do_setcontext[_tm]() to user access 
block")
Fixes: d3ccc9781560 ("powerpc/signal: Use __get_user() to copy sigset_t")
Reported-and-tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
---


Hi Finn,


Christophe, I hope this change is the one you wanted to see upstream (?).
If it is acceptable please add your signed-off-by tag.


I'm on holidays, I was planing to handle this next week.

Only PPC64 uses __get_user_sigset() on mainline so I don't think it is 
worth modifying it. If we decide to modify it anyway in mainline, it 
should be another patch that can be backported without additional effort.


For unsafe_get_user_sigset(), as we don't have the KUAP overhead that we 
had with __get_user(), I'd prefer we simply perform two 32 bits 
unsafe_get_user(), one for sig[0] and one for sig[1], instead of all 
those casts to u64.


Thanks anyway for the detailed description of the problem.
Christophe


---
  arch/powerpc/kernel/signal.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index 1f07317964e4..44e736b88e91 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -23,10 +23,10 @@ static inline int __get_user_sigset(sigset_t *dst, const 
sigset_t __user *src)
  {
BUILD_BUG_ON(sizeof(sigset_t) != sizeof(u64));
  
-	return __get_user(dst->sig[0], (u64 __user *)&src->sig[0]);

+   return __get_user(*(u64 *)&dst->sig[0], (u64 __user *)&src->sig[0]);
  }
  #define unsafe_get_user_sigset(dst, src, label) \
-   unsafe_get_user((dst)->sig[0], (u64 __user *)&(src)->sig[0], label)
+   unsafe_get_user(*(u64 *)&(dst)->sig[0], (u64 __user *)&(src)->sig[0], 
label)
  
  #ifdef CONFIG_VSX

  extern unsigned long copy_vsx_to_user(void __user *to,



[PATCH] powerpc: Fix sigset_t copy

2021-11-09 Thread Finn Thain
From: Christophe Leroy 

The conversion from __copy_from_user() to __get_user() introduced a
regression in __get_user_sigset() in v5.13. The bug was subsequently
copied and pasted in unsafe_get_user_sigset().

The regression was reported by users of the Xorg packages distributed in
Debian/powerpc --

"The symptoms are that the fb screen goes blank, with the backlight
remaining on and no errors logged in /var/log; wdm (or startx) run
with no effect (I tried logging in in the blind, with no effect).
And they are hard to kill, requiring 'kill -KILL ...'"

Fix the regression by casting the __get_user() assignment lvalue to u64
so that the entire struct gets copied.

Cc: Christophe Leroy 
Cc: Christopher M. Riedl 
Link: 
https://lore.kernel.org/linuxppc-dev/FEtBUOuFPMN4zJy4bIOqz6C4xoliCbTxS7VtMKD6UZkbvEbycUceRgGAd7e9-trRdwVN3hWAbQi0qrNx8Zgn8niTQf2KPVdw-W35czDIaeQ=@protonmail.com/
Fixes: 887f3ceb51cd ("powerpc/signal32: Convert do_setcontext[_tm]() to user 
access block")
Fixes: d3ccc9781560 ("powerpc/signal: Use __get_user() to copy sigset_t")
Reported-and-tested-by: Stan Johnson 
Signed-off-by: Finn Thain 
---
Christophe, I hope this change is the one you wanted to see upstream (?).
If it is acceptable please add your signed-off-by tag.
---
 arch/powerpc/kernel/signal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index 1f07317964e4..44e736b88e91 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -23,10 +23,10 @@ static inline int __get_user_sigset(sigset_t *dst, const 
sigset_t __user *src)
 {
BUILD_BUG_ON(sizeof(sigset_t) != sizeof(u64));
 
-   return __get_user(dst->sig[0], (u64 __user *)&src->sig[0]);
+   return __get_user(*(u64 *)&dst->sig[0], (u64 __user *)&src->sig[0]);
 }
 #define unsafe_get_user_sigset(dst, src, label) \
-   unsafe_get_user((dst)->sig[0], (u64 __user *)&(src)->sig[0], label)
+   unsafe_get_user(*(u64 *)&(dst)->sig[0], (u64 __user *)&(src)->sig[0], 
label)
 
 #ifdef CONFIG_VSX
 extern unsigned long copy_vsx_to_user(void __user *to,
-- 
2.26.3