[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAA2zVHrhiN-VUzn60Q5EO81htxR_+kJpAVcFzzkcngjKacoh2Q@mail.gmail.com>
Date: Wed, 10 Jul 2019 14:04:18 -0400
From: James Y Knight <jyknight@...gle.com>
To: musl@...ts.openwall.com
Subject: Re: [PATCH] Fix the use of sigaltstack to return to the saved
main stack.
On Tue, Jul 9, 2019 at 3:30 PM Rich Felker <dalias@...c.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@...gle.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@...gle.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
> >
>
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
