From: James Y Knight <jyknight@google.com>
To: musl@lists.openwall.com
Subject: Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack.
Date: Wed, 10 Jul 2019 14:04:18 -0400 [thread overview]
Message-ID: <CAA2zVHrhiN-VUzn60Q5EO81htxR_+kJpAVcFzzkcngjKacoh2Q@mail.gmail.com> (raw)
In-Reply-To: <20190709193004.GQ1506@brightrain.aerifal.cx>
[-- Attachment #1: Type: text/plain, Size: 5321 bytes --]
On Tue, Jul 9, 2019 at 3:30 PM Rich Felker <dalias@libc.org> wrote:
> On Tue, Jul 09, 2019 at 03:01:50PM -0400, James Y Knight wrote:
> > Previously, musl would reject the call, because the main stack has
> > ss_size == 0 and ss_flags == SS_DISABLE.
> >
> > We could condition on ss_flags not containing SS_DISABLE, but instead,
> > simply remove the ss_size check, as the kernel performs the same check,
> > anyhow.
>
> Are you sure the kernel does? I'm pretty sure the reason the code is
> here is that the kernel either does not check it, or does not perform
> the check correctly in some special case. Sadly the commit messages in
> musl were not as good back at the time when the code was written.
>
As far back as the first git version (2.6.12-rc2), the kernel checks this
condition. I haven't looked back any further...
However, I note now that musl uses different values for MINSIGSTKSIZE than
the kernel does, on some architectures.
The usual value in the kernel is 2048. Only a few architectures set a
differing value:
alpha:4096
arm64:5120
ia64:131027
sparc:4096
Musl usually uses 2048 as well, but sets other values on these
architectures:
arm64:6144
powerpc:4096
powerpc64:4096
s390x:4096
(Musl doesn't support alpha, ia64, or sparc, so it's not using a lower
value than the kernel anywhere, at least).
If it's important that stacks smaller than musl's MINSIGSTKSIZE be
rejected, despite them being large enough for the kernel, then I suppose
the check should be retained. Let me know -- I'll make a new patch
implementing your suggestion if you still think that's the way to go.
Unless we have good evidence the test isn't needed, I think the right
> check is just making the error conditional on !(ss_flags &
> SS_DISABLE). POSIX specifies: "If it is set to SS_DISABLE, the stack
> is disabled and ss_sp and ss_size are ignored." Here, "set to" is
> probably something the resolution of Austin Group issue 1187 failed to
> fix; it should probably be "includes" rather than "is set to". But I'm
> not sure it makes sense to have any flags set alongside SS_DISABLE
> anyway.
>
> [[Note: I've also included the corresponding patch to libc-tests. I'm not
> > sure if this is OK, or if I should send it separately?]]
>
> I think that's fine.
>
> Rich
>
>
>
> > From fe70a508fe945cb1a44f8a6bbd87ee295637447b Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Tue, 9 Jul 2019 14:59:01 -0400
> > Subject: [PATCH] Fix the use of sigaltstack to return to the saved main
> stack.
> >
> > Previously, musl would reject the call, because the main stack has
> > ss_size == 0 and ss_flags == SS_DISABLE.
> >
> > We could condition on ss_flags not containing SS_DISABLE, but instead,
> > simply remove the ss_size check, as the kernel performs the same check,
> > anyhow.
> > ---
> > src/signal/sigaltstack.c | 12 +++---------
> > 1 file changed, 3 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/signal/sigaltstack.c b/src/signal/sigaltstack.c
> > index cfa3f5c1..d8e8eb0b 100644
> > --- a/src/signal/sigaltstack.c
> > +++ b/src/signal/sigaltstack.c
> > @@ -4,15 +4,9 @@
> >
> > int sigaltstack(const stack_t *restrict ss, stack_t *restrict old)
> > {
> > - if (ss) {
> > - if (ss->ss_size < MINSIGSTKSZ) {
> > - errno = ENOMEM;
> > - return -1;
> > - }
> > - if (ss->ss_flags & SS_ONSTACK) {
> > - errno = EINVAL;
> > - return -1;
> > - }
> > + if (ss && (ss->ss_flags & SS_ONSTACK)) {
> > + errno = EINVAL;
> > + return -1;
> > }
> > return syscall(SYS_sigaltstack, ss, old);
> > }
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
>
> > From 9272ed8666f930dc2f24d05b1fd248584bbf495f Mon Sep 17 00:00:00 2001
> > From: James Y Knight <jyknight@google.com>
> > Date: Tue, 9 Jul 2019 14:31:24 -0400
> > Subject: [PATCH] Verify that returning to the original stack doesn't
> return an
> > error (e.g. ENOMEM).
> >
> > ---
> > src/regression/sigaltstack.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/regression/sigaltstack.c b/src/regression/sigaltstack.c
> > index bfdc44a..6847454 100644
> > --- a/src/regression/sigaltstack.c
> > +++ b/src/regression/sigaltstack.c
> > @@ -30,7 +30,7 @@ static void handler(int sig)
> >
> > int main(void)
> > {
> > - stack_t ss;
> > + stack_t ss, oldss;
> > struct sigaction sa;
> >
> > ss.ss_sp = stack;
> > @@ -39,7 +39,7 @@ int main(void)
> > sa.sa_handler = handler;
> > sa.sa_flags = SA_ONSTACK;
> >
> > - T(sigaltstack(&ss, 0));
> > + T(sigaltstack(&ss, &oldss));
> > T(sigfillset(&sa.sa_mask));
> > T(sigaction(SIGUSR1, &sa, 0));
> > T(raise(SIGUSR1));
> > @@ -56,7 +56,7 @@ int main(void)
> > t_error("sigaltstack with bad ss_flags should have failed
> with EINVAL, "
> > "got %s\n", strerror(errno));
> > errno = 0;
> > - T(sigaltstack(0, 0));
> > + T(sigaltstack(oldss, 0));
>
(And sorry -- just noticed that I forgot to commit the last fix in this
repo, before generating this patch. That should've said "&oldss".)
>
> > return t_status;
> > }
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
>
>
[-- Attachment #2: Type: text/html, Size: 7566 bytes --]
next prev parent reply other threads:[~2019-07-10 18:04 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-09 19:01 James Y Knight
2019-07-09 19:30 ` Rich Felker
2019-07-10 18:04 ` James Y Knight [this message]
2019-07-10 18:39 ` Rich Felker
2019-07-10 20:11 ` James Y Knight
2019-07-10 21:23 ` Szabolcs Nagy
2019-07-10 21:48 ` Rich Felker
2019-07-11 15:51 ` James Y Knight
2019-07-12 9:18 ` Florian Weimer
2019-07-12 16:06 ` Rich Felker
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=CAA2zVHrhiN-VUzn60Q5EO81htxR_+kJpAVcFzzkcngjKacoh2Q@mail.gmail.com \
--to=jyknight@google.com \
--cc=musl@lists.openwall.com \
/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.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
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).