> On Nov 24, 2020, at 12:47, Jann Horn <jannh@xxxxxxxxxx> wrote: > > On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok > <chang.seok.bae@xxxxxxxxx> wrote: >>> On Nov 24, 2020, at 10:41, Jann Horn <jannh@xxxxxxxxxx> wrote: >>> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok >>> <chang.seok.bae@xxxxxxxxx> wrote: >>>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@xxxxxxxxxx> wrote: >>>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@xxxxxxxxx> wrote: >>>>>> >>>>>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c >>>>>> index ee6f1ceaa7a2..cee41d684dc2 100644 >>>>>> --- a/arch/x86/kernel/signal.c >>>>>> +++ b/arch/x86/kernel/signal.c >>>>>> @@ -251,8 +251,13 @@ get_sigframe(struct k_sigaction *ka, struct pt_regs *regs, size_t frame_size, >>>>>> >>>>>> /* This is the X/Open sanctioned signal stack switching. */ >>>>>> if (ka->sa.sa_flags & SA_ONSTACK) { >>>>>> - if (sas_ss_flags(sp) == 0) >>>>>> + if (sas_ss_flags(sp) == 0) { >>>>>> + /* If the altstack might overflow, die with SIGSEGV: */ >>>>>> + if (!altstack_size_ok(current)) >>>>>> + return (void __user *)-1L; >>>>>> + >>>>>> sp = current->sas_ss_sp + current->sas_ss_size; >>>>>> + } >>>>> >>>>> A couple lines further down, we have this (since commit 14fc9fbc700d): >>>>> >>>>> /* >>>>> * If we are on the alternate signal stack and would overflow it, don't. >>>>> * Return an always-bogus address instead so we will die with SIGSEGV. >>>>> */ >>>>> if (onsigstack && !likely(on_sig_stack(sp))) >>>>> return (void __user *)-1L; >>>>> >>>>> Is that not working? >>>> >>>> onsigstack is set at the beginning here. If a signal hits under normal stack, >>>> this flag is not set. Then it will miss the overflow. >>>> >>>> The added check allows to detect the sigaltstack overflow (always). >>> >>> Ah, I think I understand what you're trying to do. But wouldn't the >>> better approach be to ensure that the existing on_sig_stack() check is >>> also used if we just switched to the signal stack? Something like: >>> >>> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c >>> index be0d7d4152ec..2f57842fb4d6 100644 >>> --- a/arch/x86/kernel/signal.c >>> +++ b/arch/x86/kernel/signal.c >>> @@ -237,7 +237,7 @@ get_sigframe(struct k_sigaction *ka, struct >>> pt_regs *regs, size_t frame_size, >>> unsigned long math_size = 0; >>> unsigned long sp = regs->sp; >>> unsigned long buf_fx = 0; >>> - int onsigstack = on_sig_stack(sp); >>> + bool onsigstack = on_sig_stack(sp); >>> int ret; >>> >>> /* redzone */ >>> @@ -246,8 +246,10 @@ get_sigframe(struct k_sigaction *ka, struct >>> pt_regs *regs, size_t frame_size, >>> >>> /* This is the X/Open sanctioned signal stack switching. */ >>> if (ka->sa.sa_flags & SA_ONSTACK) { >>> - if (sas_ss_flags(sp) == 0) >>> + if (sas_ss_flags(sp) == 0) { >>> sp = current->sas_ss_sp + current->sas_ss_size; >>> + onsigstack = true; >>> + } >>> } else if (IS_ENABLED(CONFIG_X86_32) && >>> !onsigstack && >>> regs->ss != __USER_DS && >> >> Yeah, but wouldn't it better to avoid overwriting user data if we can? The old >> check raises segfault *after* overwritten. > > Where is that overwrite happening? Between the point where your check > happens, and the point where the old check is, the only calls are to > fpu__alloc_mathframe() and align_sigframe(), right? > fpu__alloc_mathframe() just does some size calculations and doesn't > write anything. align_sigframe() also just does size calculations. Am > I missing something? Yeah, you’re right. Right now, I’m thinking your approach is simpler and providing almost the same function (unless I’m missing here). Thanks, Chang