Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20230907194935.GG3448312@port70.net>
Date: Thu, 7 Sep 2023 21:49:35 +0200
From: Szabolcs Nagy <nsz@...t70.net>
To: Rich Felker <dalias@...c.org>
Cc: musl@...ts.openwall.com, Peter Williams <peter@...ton.cx>
Subject: Re: aarch64 sigsetjmp relocation truncation bug, maybe

* Rich Felker <dalias@...c.org> [2023-09-07 09:28:52 -0400]:
> On Thu, Sep 07, 2023 at 08:48:28AM -0400, Rich Felker wrote:
> > On Thu, Sep 07, 2023 at 05:08:13AM +0200, Markus Wichmann wrote:
> > > Am Wed, Sep 06, 2023 at 08:46:32PM -0400 schrieb Peter Williams:
> > > > If I'm understanding correctly, the complaint is that a branch in
> > > > sigsetjmp that invokes setjmp is too far away from the definition of
> > > > setjmp. My very handwavey idea is that maybe for some reason my program
> > > > is causing the linker to want to locate setjmp() and sigsetjmp() really
> > > > far away from each other. If that's right, perhaps it would be possible
> > > > to modify the assembler code to be able to handle such a situation?
> > > 
> > > I'm guessing the same. Pretty much all architectures have shorter
> > > conditional than unconditional branches. That is why branches to other
> > > files (technically to other sections) should always be unconditional. I
> > > am attaching a simple patch that should help with the situation.
> > > 
> > > Ciao,
> > > Markus
> > 
> > > From dd227e22a5337d54e1cb0838410bca6672c76c43 Mon Sep 17 00:00:00 2001
> > > From: Markus Wichmann <nullplan@....net>
> > > Date: Thu, 7 Sep 2023 05:01:23 +0200
> > > Subject: [PATCH] Make branch to external symbol unconditional.
> > > 
> > > Conditional branches have a shorter branch length than unconditional
> > > ones, and almost all ABIs require unconditional branches to external
> > > symbols. Otherwise linkers may create broken binaries.
> > > ---
> > >  src/signal/aarch64/sigsetjmp.s | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/signal/aarch64/sigsetjmp.s b/src/signal/aarch64/sigsetjmp..s
> > > index 75910c43..9a28e395 100644
> > > --- a/src/signal/aarch64/sigsetjmp.s
> > > +++ b/src/signal/aarch64/sigsetjmp.s
> > > @@ -4,7 +4,7 @@
> > >  .type __sigsetjmp,%function
> > >  sigsetjmp:
> > >  __sigsetjmp:
> > > -	cbz x1,setjmp
> > > +	cbz x1,1f
> > > 
> > >  	str x30,[x0,#176]
> > >  	str x19,[x0,#176+8+8]
> > > @@ -19,3 +19,5 @@ __sigsetjmp:
> > > 
> > >  .hidden __sigsetjmp_tail
> > >  	b __sigsetjmp_tail
> > > +
> > > +1: b setjmp

yeah, this looks good except the commit message can be improved.

conditional branch has +-1M reach, unconditional branch has +-128M
reach and the linker inserts a veneer if unconditional branch goes
further than that (i.e. it can go arbitrarily far).

setjmp is a local symbol to libc (or in case of static linking to
the exe), so using unconditional branch is perfectly fine if we
ensure the linker does not move the symbols far (e.g. putting the
two symbols in the same object file or into the same small section
is enough). this depends on that user code cannot interpose setjmp
(link namespace violation) and setjmp in libc does not use plt.

note that aarch64 libc.so text is < 1M so we can compile it with
-mcmodel=tiny code model and then the compiler would emit cond
branch for libc symbols. but .lo objects built like that are not
suitable for libc.a.

the simple b setjmp is the reasonable fix for now.

> > Are you sure this is the actual problem? I think it's that the aarch64
> > (and several other archs) version of sigsetjmp is wrongly using the
> > public setjmp symbol whose definition is possibly provided by a PLT
> > thunk in the main program, rather than either setjmp@PLT (which would
> > necessarily be the right local call point to use) or the hidden
> > ___setjmp symbol that exists for this purpose (which i386, for
> > example, uses).
> 
> Hmm, no, that seems to be a separate issue. The reported issue is
> indeed a large static link where PLT stuff should not come into play.
> So I think the cbz limited-range is indeed the issue.
> 
> Rich

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.