* Replacing a_crash() ? @ 2018-09-17 3:23 Rich Felker 2018-09-17 3:50 ` A. Wilcox ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Rich Felker @ 2018-09-17 3:23 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Replacing a_crash() ? 2018-09-17 3:23 Replacing a_crash() ? Rich Felker @ 2018-09-17 3:50 ` A. Wilcox 2018-09-17 9:24 ` u-uy74 2018-09-17 11:13 ` Szabolcs Nagy 2018-09-17 15:24 ` Markus Wichmann 2 siblings, 1 reply; 7+ messages in thread From: A. Wilcox @ 2018-09-17 3:50 UTC (permalink / raw) To: musl [-- 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 --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Replacing a_crash() ? 2018-09-17 3:50 ` A. Wilcox @ 2018-09-17 9:24 ` u-uy74 0 siblings, 0 replies; 7+ messages in thread From: u-uy74 @ 2018-09-17 9:24 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Replacing a_crash() ? 2018-09-17 3:23 Replacing a_crash() ? Rich Felker 2018-09-17 3:50 ` A. Wilcox @ 2018-09-17 11:13 ` Szabolcs Nagy 2018-09-17 13:24 ` Rich Felker 2018-09-17 15:24 ` Markus Wichmann 2 siblings, 1 reply; 7+ messages in thread From: Szabolcs Nagy @ 2018-09-17 11:13 UTC (permalink / raw) To: musl * 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..) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Replacing a_crash() ? 2018-09-17 11:13 ` Szabolcs Nagy @ 2018-09-17 13:24 ` Rich Felker 0 siblings, 0 replies; 7+ messages in thread From: Rich Felker @ 2018-09-17 13:24 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Replacing a_crash() ? 2018-09-17 3:23 Replacing a_crash() ? Rich Felker 2018-09-17 3:50 ` A. Wilcox 2018-09-17 11:13 ` Szabolcs Nagy @ 2018-09-17 15:24 ` Markus Wichmann 2018-09-17 15:36 ` Rich Felker 2 siblings, 1 reply; 7+ messages in thread From: Markus Wichmann @ 2018-09-17 15:24 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Replacing a_crash() ? 2018-09-17 15:24 ` Markus Wichmann @ 2018-09-17 15:36 ` Rich Felker 0 siblings, 0 replies; 7+ messages in thread From: Rich Felker @ 2018-09-17 15:36 UTC (permalink / raw) To: musl 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-17 15:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-17 3:23 Replacing a_crash() ? Rich Felker 2018-09-17 3:50 ` A. Wilcox 2018-09-17 9:24 ` u-uy74 2018-09-17 11:13 ` Szabolcs Nagy 2018-09-17 13:24 ` Rich Felker 2018-09-17 15:24 ` Markus Wichmann 2018-09-17 15:36 ` Rich Felker
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).