From: "Bae, Chang Seok" <chang.seok.bae@intel.com>
To: Jann Horn <jannh@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@suse.de>,
Andy Lutomirski <luto@kernel.org>,
"the arch/x86 maintainers" <x86@kernel.org>,
"Brown, Len" <len.brown@intel.com>,
"Hansen, Dave" <dave.hansen@intel.com>,
"H.J. Lu" <hjl.tools@gmail.com>,
Dave Martin <Dave.Martin@arm.com>,
Michael Ellerman <mpe@ellerman.id.au>,
"Luck, Tony" <tony.luck@intel.com>,
"Shankar, Ravi V" <ravi.v.shankar@intel.com>,
"libc-alpha@sourceware.org" <libc-alpha@sourceware.org>,
linux-arch <linux-arch@vger.kernel.org>,
"Linux API" <linux-api@vger.kernel.org>,
kernel list <linux-kernel@vger.kernel.org>,
Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
Subject: Re: [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery
Date: Tue, 24 Nov 2020 20:55:27 +0000 [thread overview]
Message-ID: <00730AF2-9727-4BA6-8C2A-164BD38738F1@intel.com> (raw)
In-Reply-To: <CAG48ez3=0P+yiAjxGy=uEZeDUvFh+M2GUnVaGPfRoQHbJ+2qKw@mail.gmail.com>
> On Nov 24, 2020, at 12:47, Jann Horn <jannh@google.com> wrote:
>
> On Tue, Nov 24, 2020 at 9:43 PM Bae, Chang Seok
> <chang.seok.bae@intel.com> wrote:
>>> On Nov 24, 2020, at 10:41, Jann Horn <jannh@google.com> wrote:
>>> On Tue, Nov 24, 2020 at 7:22 PM Bae, Chang Seok
>>> <chang.seok.bae@intel.com> wrote:
>>>>> On Nov 20, 2020, at 15:04, Jann Horn <jannh@google.com> wrote:
>>>>> On Thu, Nov 19, 2020 at 8:40 PM Chang S. Bae <chang.seok.bae@intel.com> 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
next prev parent reply other threads:[~2020-11-24 20:56 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-19 19:02 [PATCH v2 0/4] x86: Improve Minimum Alternate Stack Size Chang S. Bae
2020-11-19 19:02 ` [PATCH v2 1/4] x86/signal: Introduce helpers to get the maximum signal frame size Chang S. Bae
2020-11-25 11:17 ` Borislav Petkov
2020-11-30 20:40 ` Bae, Chang Seok
2020-12-01 14:27 ` Borislav Petkov
2020-11-19 19:02 ` [PATCH v2 2/4] x86/elf: Support a new ELF aux vector AT_MINSIGSTKSZ Chang S. Bae
2020-11-26 17:44 ` Borislav Petkov
2020-11-27 9:30 ` Michael Kerrisk (man-pages)
2020-11-19 19:02 ` [PATCH v2 3/4] x86/signal: Prevent an alternate stack overflow before a signal delivery Chang S. Bae
2020-11-20 23:04 ` Jann Horn
2020-11-24 18:22 ` Bae, Chang Seok
2020-11-24 18:41 ` Jann Horn
2020-11-24 20:43 ` Bae, Chang Seok
2020-11-24 20:47 ` Jann Horn
2020-11-24 20:55 ` Bae, Chang Seok [this message]
2021-02-08 20:29 ` Bae, Chang Seok
2020-11-19 19:02 ` [PATCH v2 4/4] selftest/x86/signal: Include test cases for validating sigaltstack Chang S. Bae
2020-11-27 17:32 ` Borislav Petkov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=00730AF2-9727-4BA6-8C2A-164BD38738F1@intel.com \
--to=chang.seok.bae@intel.com \
--cc=Dave.Martin@arm.com \
--cc=bp@suse.de \
--cc=dave.hansen@intel.com \
--cc=h-shimamoto@ct.jp.nec.com \
--cc=hjl.tools@gmail.com \
--cc=jannh@google.com \
--cc=len.brown@intel.com \
--cc=libc-alpha@sourceware.org \
--cc=linux-api@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@kernel.org \
--cc=mpe@ellerman.id.au \
--cc=ravi.v.shankar@intel.com \
--cc=tglx@linutronix.de \
--cc=tony.luck@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).