Replacing a_crash() ?

Replacing a_crash() ?

[Rich Felker]

Now that we have an abort() that reliably terminates with uncatchable
SIGABRT, I've been thinking about replacing the a_crash() calls in
musl (which are usually an instruction generating SIGILL or SIGSEGV)
with calls to the uncatchable tail of abort(), which I would factor
off as a __forced_abort() function.

In case it's not clear, the reason for not just calling abort() is
that too many programs catch it, and catching it is even encouraged.
Catchability is a problem with the current approach too, since
a_crash() is used in places where process state is known to be
dangerously corrupt and likely under attacker control; eliminating it
is one of the potential goals of switching to __forced_abort().

Are there any objections to making such a change? So far I've gotten
mostly positive feedback -- SIGABRT is more telling of what's happened
than SIGSEGV/SIGILL. It would also get rid of the ugly misplacement of
a_crash() (no longer needed) in "atomic.h" and the inclusion of
"atomic.h" in some files where it makes no sense without knowing it's
where a_crash() is defined.

For i386, some nontrivial work would be needed to make abort's tail
perform syscalls with int $128 rather than the vdso, which is unsafe
since the pointer to it may have been subverted. On other archs,
inline syscalls are fully inline. I'd probably add a
NEED_FAILSAFE_SYSCALL macro to define before including "syscall.h" and
have arch/i386/syscall_arch.h adjust the asm string based on it; this
is more maintainable than writing an asm version of the function.

Rich

Re: Replacing a_crash() ?

[A. Wilcox]

[-- Attachment #1.1: Type: text/plain, Size: 1828 bytes --]

On 09/16/18 22:23, Rich Felker wrote:
> Now that we have an abort() that reliably terminates with uncatchable
> SIGABRT, I've been thinking about replacing the a_crash() calls in
> musl (which are usually an instruction generating SIGILL or SIGSEGV)
> with calls to the uncatchable tail of abort(), which I would factor
> off as a __forced_abort() function.
> 
> In case it's not clear, the reason for not just calling abort() is
> that too many programs catch it, and catching it is even encouraged.
> Catchability is a problem with the current approach too, since
> a_crash() is used in places where process state is known to be
> dangerously corrupt and likely under attacker control; eliminating it
> is one of the potential goals of switching to __forced_abort().


Yes, please!


> Are there any objections to making such a change? So far I've gotten
> mostly positive feedback -- SIGABRT is more telling of what's happened
> than SIGSEGV/SIGILL. It would also get rid of the ugly misplacement of
> a_crash() (no longer needed) in "atomic.h" and the inclusion of
> "atomic.h" in some files where it makes no sense without knowing it's
> where a_crash() is defined.
> 
> For i386, some nontrivial work would be needed to make abort's tail
> perform syscalls with int $128 rather than the vdso, which is unsafe
> since the pointer to it may have been subverted. On other archs,
> inline syscalls are fully inline. I'd probably add a
> NEED_FAILSAFE_SYSCALL macro to define before including "syscall.h" and
> have arch/i386/syscall_arch.h adjust the asm string based on it; this
> is more maintainable than writing an asm version of the function.


That seems like a sane way to do it.

Best,
--arw

-- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
https://www.adelielinux.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Replacing a_crash() ?

[u-uy74]

On Sun, Sep 16, 2018 at 10:50:26PM -0500, A. Wilcox wrote:
 ...
> Yes, please!
 ...
> That seems like a sane way to do it.

+1

Rune

Re: Replacing a_crash() ?

[Szabolcs Nagy]

* Rich Felker <dalias@libc.org> [2018-09-16 23:23:17 -0400]:
> Now that we have an abort() that reliably terminates with uncatchable
> SIGABRT, I've been thinking about replacing the a_crash() calls in
> musl (which are usually an instruction generating SIGILL or SIGSEGV)
> with calls to the uncatchable tail of abort(), which I would factor
> off as a __forced_abort() function.
> 
> In case it's not clear, the reason for not just calling abort() is
> that too many programs catch it, and catching it is even encouraged.
> Catchability is a problem with the current approach too, since
> a_crash() is used in places where process state is known to be
> dangerously corrupt and likely under attacker control; eliminating it
> is one of the potential goals of switching to __forced_abort().
> 

i wonder if it can be made debugging friendly in some way,
e.g. with multiple failure paths merged into a single
__forced_abort call or when it's tail called, it may
not be clear from a core dump why the abort happened.

if __forced_abort(const char *reason) stored its argument
somewhere that is not clobbered then it may be easier to
figure out what went wrong. (you would still need some
debug skills to look at the reason though..)

Re: Replacing a_crash() ?

[Rich Felker]

On Mon, Sep 17, 2018 at 01:13:50PM +0200, Szabolcs Nagy wrote:
> * Rich Felker <dalias@libc.org> [2018-09-16 23:23:17 -0400]:
> > Now that we have an abort() that reliably terminates with uncatchable
> > SIGABRT, I've been thinking about replacing the a_crash() calls in
> > musl (which are usually an instruction generating SIGILL or SIGSEGV)
> > with calls to the uncatchable tail of abort(), which I would factor
> > off as a __forced_abort() function.
> > 
> > In case it's not clear, the reason for not just calling abort() is
> > that too many programs catch it, and catching it is even encouraged.
> > Catchability is a problem with the current approach too, since
> > a_crash() is used in places where process state is known to be
> > dangerously corrupt and likely under attacker control; eliminating it
> > is one of the potential goals of switching to __forced_abort().
> 
> i wonder if it can be made debugging friendly in some way,
> e.g. with multiple failure paths merged into a single
> __forced_abort call or when it's tail called, it may
> not be clear from a core dump why the abort happened.
> 
> if __forced_abort(const char *reason) stored its argument
> somewhere that is not clobbered then it may be easier to
> figure out what went wrong. (you would still need some
> debug skills to look at the reason though..)

I think gcc already does something to make _Noreturn functions easier
to debug like this, doesn't it? There's really not much advantage to a
tail call when the function won't return.

Rich

Re: Replacing a_crash() ?

[Markus Wichmann]

On Sun, Sep 16, 2018 at 11:23:17PM -0400, Rich Felker wrote:
> Now that we have an abort() that reliably terminates with uncatchable
> SIGABRT, I've been thinking about replacing the a_crash() calls in
> musl (which are usually an instruction generating SIGILL or SIGSEGV)
> with calls to the uncatchable tail of abort(), which I would factor
> off as a __forced_abort() function.
> 
> In case it's not clear, the reason for not just calling abort() is
> that too many programs catch it, and catching it is even encouraged.
> Catchability is a problem with the current approach too, since
> a_crash() is used in places where process state is known to be
> dangerously corrupt and likely under attacker control; eliminating it
> is one of the potential goals of switching to __forced_abort().
> 
> Are there any objections to making such a change? So far I've gotten
> mostly positive feedback -- SIGABRT is more telling of what's happened
> than SIGSEGV/SIGILL. It would also get rid of the ugly misplacement of
> a_crash() (no longer needed) in "atomic.h" and the inclusion of
> "atomic.h" in some files where it makes no sense without knowing it's
> where a_crash() is defined.
> 
> For i386, some nontrivial work would be needed to make abort's tail
> perform syscalls with int $128 rather than the vdso, which is unsafe
> since the pointer to it may have been subverted. On other archs,
> inline syscalls are fully inline. I'd probably add a
> NEED_FAILSAFE_SYSCALL macro to define before including "syscall.h" and
> have arch/i386/syscall_arch.h adjust the asm string based on it; this
> is more maintainable than writing an asm version of the function.
> 
> Rich

Simple checklist for whether to perform a change or not:

1. Does the change fix problems? Check (namely, maintainability,
legibility, understandability of problems).

2. Does the change introduce problems? Unlikely. Someone might subvert
__forced_abort(), but then, someone might catch SIGILL, so we haven't
gone anywhere.

3. Is the change compatible with old programs? No, but a_crash() was
never a defined interface, so any program catching it was walking on
thin ice, anyway.

So that's two green lights and a don't care, so please go ahead.

Ciao,
Markus

Re: Replacing a_crash() ?

[Rich Felker]

On Mon, Sep 17, 2018 at 05:24:15PM +0200, Markus Wichmann wrote:
> On Sun, Sep 16, 2018 at 11:23:17PM -0400, Rich Felker wrote:
> > Now that we have an abort() that reliably terminates with uncatchable
> > SIGABRT, I've been thinking about replacing the a_crash() calls in
> > musl (which are usually an instruction generating SIGILL or SIGSEGV)
> > with calls to the uncatchable tail of abort(), which I would factor
> > off as a __forced_abort() function.
> > 
> > In case it's not clear, the reason for not just calling abort() is
> > that too many programs catch it, and catching it is even encouraged.
> > Catchability is a problem with the current approach too, since
> > a_crash() is used in places where process state is known to be
> > dangerously corrupt and likely under attacker control; eliminating it
> > is one of the potential goals of switching to __forced_abort().
> > 
> > Are there any objections to making such a change? So far I've gotten
> > mostly positive feedback -- SIGABRT is more telling of what's happened
> > than SIGSEGV/SIGILL. It would also get rid of the ugly misplacement of
> > a_crash() (no longer needed) in "atomic.h" and the inclusion of
> > "atomic.h" in some files where it makes no sense without knowing it's
> > where a_crash() is defined.
> > 
> > For i386, some nontrivial work would be needed to make abort's tail
> > perform syscalls with int $128 rather than the vdso, which is unsafe
> > since the pointer to it may have been subverted. On other archs,
> > inline syscalls are fully inline. I'd probably add a
> > NEED_FAILSAFE_SYSCALL macro to define before including "syscall.h" and
> > have arch/i386/syscall_arch.h adjust the asm string based on it; this
> > is more maintainable than writing an asm version of the function.
> 
> Simple checklist for whether to perform a change or not:
> 
> 1. Does the change fix problems? Check (namely, maintainability,
> legibility, understandability of problems).

It slightly reduces amount of per-arch asm needed. (Actually not,
because there's a "generic" a_crash() that writes to a volatile null
pointer, but it doesn't work on nommu.) It also gets rid of atomic.h
dependencies.

> 2. Does the change introduce problems? Unlikely. Someone might subvert
> __forced_abort(), but then, someone might catch SIGILL, so we haven't
> gone anywhere.

I was thinking more like friendliness to debugging workflows; that's
the motivation for not using SIGKILL, which always would have been
easy. Subverting __forced_abort() for static linking is of course
easy; for dynamic you'd have to modify the mapped libc.so since it
would be a direct call.

> 3. Is the change compatible with old programs? No, but a_crash() was
> never a defined interface, so any program catching it was walking on
> thin ice, anyway.
> 
> So that's two green lights and a don't care, so please go ahead.

Indeed, catching it was never intended to be supported usage.

Rich