| Message ID | 08bbe7240b384016e0b2912ecf3bf5e2d25ef2c6.1636501628.git.fthain@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: Fix sigset_t copy | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
Le 10/11/2021 à 00:47, Finn Thain a écrit : > From: Christophe Leroy <christophe.leroy@csgroup.eu> > > 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 <christophe.leroy@csgroup.eu> > Cc: Christopher M. Riedl <cmr@bluescreens.de> > 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 <userm57@yahoo.com> > Signed-off-by: Finn Thain <fthain@linux-m68k.org> > --- 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, >
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.
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,