* [PATCH] powerpc: Fix sigset_t copy
@ 2021-11-09 23:47 Finn Thain
2021-11-10 17:12 ` Christophe Leroy
0 siblings, 1 reply; 3+ messages in thread
From: Finn Thain @ 2021-11-09 23:47 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Christophe Leroy
Cc: linuxppc-dev, linux-kernel, Christopher M. Riedl
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>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc: Fix sigset_t copy
2021-11-09 23:47 [PATCH] powerpc: Fix sigset_t copy Finn Thain
@ 2021-11-10 17:12 ` Christophe Leroy
2021-11-11 0:20 ` Finn Thain
0 siblings, 1 reply; 3+ messages in thread
From: Christophe Leroy @ 2021-11-10 17:12 UTC (permalink / raw)
To: Finn Thain, Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel, Christopher M. Riedl
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,
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc: Fix sigset_t copy
2021-11-10 17:12 ` Christophe Leroy
@ 2021-11-11 0:20 ` Finn Thain
0 siblings, 0 replies; 3+ messages in thread
From: Finn Thain @ 2021-11-11 0:20 UTC (permalink / raw)
To: Christophe Leroy
Cc: linux-kernel, Paul Mackerras, Christopher M. Riedl, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 339 bytes --]
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-11 0:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-09 23:47 [PATCH] powerpc: Fix sigset_t copy Finn Thain
2021-11-10 17:12 ` Christophe Leroy
2021-11-11 0:20 ` Finn Thain
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).