From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/14379 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: James Y Knight Newsgroups: gmane.linux.lib.musl.general Subject: Re: [PATCH] Fix the use of sigaltstack to return to the saved main stack. Date: Wed, 10 Jul 2019 14:04:18 -0400 Message-ID: References: <20190709193004.GQ1506@brightrain.aerifal.cx> Reply-To: musl@lists.openwall.com Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="00000000000023a9eb058d5783e4" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="111356"; mail-complaints-to="usenet@blaine.gmane.org" To: musl@lists.openwall.com Original-X-From: musl-return-14395-gllmg-musl=m.gmane.org@lists.openwall.com Wed Jul 10 20:05:03 2019 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.89) (envelope-from ) id 1hlGxi-000SsF-Kp for gllmg-musl@m.gmane.org; Wed, 10 Jul 2019 20:05:02 +0200 Original-Received: (qmail 13513 invoked by uid 550); 10 Jul 2019 18:04:58 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 13491 invoked from network); 10 Jul 2019 18:04:58 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=JaC262XzlVjbKADXJY0Dx0Q5kOJ165JQU2xyuNSrqP8=; b=FRWTpCwbWmF1mGeEIyCfUIeCdd6Y74MPh6eard9hQIUqFk15CJWhHcUxt7Q1IaJXlr /haPYAEQ78OtxMmGPN4gHCdvKiy0EB/jutZr8JZ8kUb3/r2Ey/ZoEZqM4fiDy/xqQDYZ UYdrluzNsK+jpbULiDOVt7kmX60BkWLuuy9Wf7l7oz1exhzLue0owk0XUHfh2Blc0PZq lPB4dcwzl76DonUxfcGHznqQ+/kv8d10gwRzdWvnGqANQjgeXn7meAQbvtQGrkI/IT63 oFvBOsAtndHXA095HqwkYZjf6HcjgPcNLTl/nUitRLqu2KKhyXFr+JreEPd6s1p/ktXG vCXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=JaC262XzlVjbKADXJY0Dx0Q5kOJ165JQU2xyuNSrqP8=; b=m4LNV4glDuSsKeKn6+tCf7TqnAHkB5LEtemGHOvdQYByfzwris1nHFO9V/2FZ0/GYp jHE3KM/+JInW4GDrVUSsP+vboPkKcwndZdiombpxMyAqCDV5SM+pM6I1hfeTLhcwY+gH zST25Beieksew6xBiDni7TGt1PnyHvYHI96wL2YoWnkvHW8G+1pHqjShu5WefBrqrUKJ w6vfCvhC3ldB8i+aM/lqEKPaucTHsV/ZD43i+ZmDR0lcWR6yUQ22LeU5le+4Wdgbnq/A 6BJe6DU+UOfwYw1yKT9Cafsjq9hVwhMs1YYiZl2wlJ7Hn/as92Kq7LinXihEhIuHCgcP HbBQ== X-Gm-Message-State: APjAAAUmP24s4ofNzKcU8gqe2a5XiV0ZBOjuZ7jEiVA9/U6cM1sCaJX2 M32Ag+zUZLHTSs2KcRofIU9pB7H6my30AGHe4Ts8ehHa6TE= X-Google-Smtp-Source: APXvYqynLCNsiT1+Qj4bxhOKbW1RimObxeUIIjfOT+h4UJ6C19U7olgErwJ5KKTmSDcA42xKGl/RPEo+gjiUfEPGCX8= X-Received: by 2002:ab0:66:: with SMTP id 93mr18021727uai.135.1562781885350; Wed, 10 Jul 2019 11:04:45 -0700 (PDT) In-Reply-To: <20190709193004.GQ1506@brightrain.aerifal.cx> Xref: news.gmane.org gmane.linux.lib.musl.general:14379 Archived-At: --00000000000023a9eb058d5783e4 Content-Type: text/plain; charset="UTF-8" On Tue, Jul 9, 2019 at 3:30 PM Rich Felker 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 > > 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 > > 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 > > > > --00000000000023a9eb058d5783e4 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Jul 9, 2019 at 3:30 PM Rich F= elker <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 =3D=3D 0 and ss_flags =3D=3D 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 va= lues=C2=A0for MINSIGSTKSIZE=C2=A0than the kernel does, on some architecture= s.

The usual value in the kernel is 2048. Only a f= ew architectures set a differing value:
alpha:4096
arm6= 4:5120
ia64:131027
sparc:4096

Musl usual= ly 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 kerne= l, then I suppose the check should be retained. Let me know -- I'll mak= e a new patch implementing your suggestion if you still think that's th= e 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&quo= t; 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.
=C2= =A0
> [[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 mai= n stack.
>
> Previously, musl would reject the call, because the main stack has
> ss_size =3D=3D 0 and ss_flags =3D=3D 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.
> ---
>=C2=A0 src/signal/sigaltstack.c | 12 +++---------
>=C2=A0 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 @@
>=C2=A0
>=C2=A0 int sigaltstack(const stack_t *restrict ss, stack_t *restrict ol= d)
>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0if (ss) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ss->ss_size &l= t; MINSIGSTKSZ) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0errno =3D ENOMEM;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return -1;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (ss->ss_flags &= amp; SS_ONSTACK) {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0errno =3D EINVAL;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0return -1;
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0}
> +=C2=A0 =C2=A0 =C2=A0if (ss && (ss->ss_flags & SS_ONSTA= CK)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0errno =3D EINVAL;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -1;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0}
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return syscall(SYS_sigaltstack, ss, old); >=C2=A0 }
> --
> 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
>=C2=A0 error (e.g. ENOMEM).
>
> ---
>=C2=A0 src/regression/sigaltstack.c | 6 +++---
>=C2=A0 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)
>=C2=A0
>=C2=A0 int main(void)
>=C2=A0 {
> -=C2=A0 =C2=A0 =C2=A0stack_t ss;
> +=C2=A0 =C2=A0 =C2=A0stack_t ss, oldss;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0struct sigaction sa;
>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0ss.ss_sp =3D stack;
> @@ -39,7 +39,7 @@ int main(void)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0sa.sa_handler =3D handler;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0sa.sa_flags =3D SA_ONSTACK;
>=C2=A0
> -=C2=A0 =C2=A0 =C2=A0T(sigaltstack(&ss, 0));
> +=C2=A0 =C2=A0 =C2=A0T(sigaltstack(&ss, &oldss));
>=C2=A0 =C2=A0 =C2=A0 =C2=A0T(sigfillset(&sa.sa_mask));
>=C2=A0 =C2=A0 =C2=A0 =C2=A0T(sigaction(SIGUSR1, &sa, 0));
>=C2=A0 =C2=A0 =C2=A0 =C2=A0T(raise(SIGUSR1));
> @@ -56,7 +56,7 @@ int main(void)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0t_error("si= galtstack with bad ss_flags should have failed with EINVAL, "
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0"got %s\n", strerror(errno));
>=C2=A0 =C2=A0 =C2=A0 =C2=A0errno =3D 0;
> -=C2=A0 =C2=A0 =C2=A0T(sigaltstack(0, 0));
> +=C2=A0 =C2=A0 =C2=A0T(sigaltstack(oldss, 0));
(And sorry -- just noticed that I forgot to commit the last fi= x in this repo, before generating this patch. That should've said "= ;&oldss".)

>=C2=A0
>=C2=A0 =C2=A0 =C2=A0 =C2=A0return t_status;
>=C2=A0 }
> --
> 2.22.0.410.gd8fdbe21b5-goog
>

--00000000000023a9eb058d5783e4--