Re: [PATCH 0/3] signal: HANDLER_EXIT should clear SIGNAL_UNKILLABLE

[Eric W. Biederman]

Kees Cook <keescook@xxxxxxxxxxxx> writes:

> On Thu, Feb 10, 2022 at 04:48:30PM -0600, Eric W. Biederman wrote:
>> Kees Cook <keescook@xxxxxxxxxxxx> writes:
>> 
>> > On Thu, Feb 10, 2022 at 12:58:07PM -0600, Eric W. Biederman wrote:
>> >> Kees Cook <keescook@xxxxxxxxxxxx> writes:
>> >> 
>> >> > On Thu, Feb 10, 2022 at 12:17:50PM -0600, Eric W. Biederman wrote:
>> >> >> Kees Cook <keescook@xxxxxxxxxxxx> writes:
>> >> >> 
>> >> >> > Hi,
>> >> >> >
>> >> >> > This fixes the signal refactoring to actually kill unkillable processes
>> >> >> > when receiving a fatal SIGSYS from seccomp. Thanks to Robert for the
>> >> >> > report and Eric for the fix! I've also tweaked seccomp internal a bit to
>> >> >> > fail more safely. This was a partial seccomp bypass, in the sense that
>> >> >> > SECCOMP_RET_KILL_* didn't kill the process, but it didn't bypass other
>> >> >> > aspects of the filters. (i.e. the syscall was still blocked, etc.)
>> >> >> 
>> >> >> Any luck on figuring out how to suppress the extra event?
>> >> >
>> >> > I haven't found a good single indicator of a process being in an "I am dying"
>> >> > state, and even if I did, it seems every architecture's exit path would
>> >> > need to add a new test.
>> >> 
>> >> The "I am dying" state for a task is fatal_signal_pending, at least
>> >> before get_signal is reached, for a process there is SIGNAL_GROUP_EXIT.
>> >> Something I am busily cleaning up and making more reliable at the
>> >> moment.
>> >
>> > The state I need to catch is "I am dying and this syscall was
>> > interrupted". fatal_signal_pending() is kind of only the first half
>> > (though it doesn't cover fatal SIGSYS?)
>> >
>> > For example, if a process hits a BUG() in the middle of running a
>> > syscall, that syscall isn't expected to "exit" from the perspective of
>> > userspace. This is similarly true for seccomp's fatal SIGSYS.
>> >
>> >> What is the event that is happening?  Is it
>> >> tracehook_report_syscall_exit or something else?
>> >
>> > Yes, but in more completely, it's these three, which are called in
>> > various fashions from architecture syscall exit code:
>> >
>> > 	audit_syscall_exit()		(audit)
>> > 	trace_sys_exit()		(see "TRACE_EVENT_FN(sys_exit,")
>> > 	tracehook_report_syscall_exit()	(ptrace)
>> >
>> >> From the bits I have seen it seems like something else.
>> >
>> > But yes, the place Robert and I both noticed it was with ptrace from
>> > tracehook_report_syscall_exit(), which is rather poorly named. :)
>> 
>> Speaking of patches I am just about to send out.
>> 
>> > Looking at the results, audit_syscall_exit() and trace_sys_exit() need
>> > to be skipped too, since they would each be reporting potential nonsense.
>> >
>> >> > The best approach seems to be clearing the TIF_*WORK* bits, but that's
>> >> > still a bit arch-specific. And I'm not sure which layer would do that.
>> >> > At what point have we decided the process will not continue? More
>> >> > than seccomp was calling do_exit() in the middle of a syscall, but those
>> >> > appear to have all been either SIGKILL or SIGSEGV?
>> >> 
>> >> This is where I get confused what TIF_WORK bits matter?
>> >
>> > This is where I wish all the architectures were using the common syscall
>> > code. The old do_exit() path would completely skip _everything_ in the
>> > exit path, so it was like never calling anything after the syscall
>> > dispatch table. The only userspace visible things in there are triggered
>> > from having TIF_WORK... flags (but again, it's kind of a per-arch mess).
>> >
>> > Skipping the entire exit path makes a fair bit of sense. For example,
>> > rseq_syscall() is redundant (forcing SIGSEGV).
>> >
>> > Regardless, at least the three places above need to be skipped.
>> >
>> > But just testing fatal_signal_pending() seems wrong: a normal syscall
>> > could be finishing just fine, it just happens to have a fatal signal
>> > ready to be processed.
>> 
>> Yes.  It is really just the HANDLER_EXIT case where this is interesting.
>
> Right.
>
>> 
>> >
>> > Here's the ordering after a syscall on x86 from do_syscall_64():
>> >
>> > do_syscall_x64()
>> > 	sys_call_table[...](regs)
>> > syscall_exit_to_user_mode()
>> > 	__syscall_exit_to_user_mode_work()
>> > 		syscall_exit_to_user_mode_prepare()
>> > 			syscall_exit_work()
>> > 				arch_syscall_exit_tracehook()
>> > 					tracehook_report_syscall_exit()
>> > 	exit_to_user_mode_prepare()
>> > 		exit_to_user_mode_loop()
>> > 			handle_signal_work()
>> > 				arch_do_signal_or_restart()
>> > 					get_signal()
>> > 						do_group_exit()
>> >
>> > Here's arm64 from el0_svc():
>> >
>> > do_el0_svc()
>> > 	el0_svc_common()
>> > 		invoke_syscall()
>> > 			syscall_table[...](regs)
>> > 		syscall_trace_exit()
>> > 			tracehook_report_syscall()
>> > 				tracehook_report_syscall_exit()
>> > exit_to_user_mode()
>> > 	prepare_exit_to_user_mode()
>> > 		do_notify_resume()
>> > 			do_signal()
>> > 				get_signal()
>> > 					do_group_exit()
>> >
>> > In the past, any do_exit() would short circuit everything after the
>> > syscall table. Now, we do all the exit work before starting the return
>> > to user mode which is what processes the signals. So I guess there's
>> > more precisely a difference between "visible to userspace" and "return
>> > to userspace".
>> 
>> Yes.  I see that now.  I had not had an occasion to look at the order
>> all of these were called in before and my mental model was wrong.
>
> Yeah, I didn't even have a model of this all the way. I'd really only
> understood the ptrace side of it.
>
>> It makes a certain kind of sense that the per syscall work happens
>> before we do additional things like process signals.  It simply
>> had not realized that was happening in that order until now.
>> 
>> 
>> > (an aside: where to PF_IO_WORKER threads die?)
>> 
>> They are calling do_exit explicitly.
>
> Ah-ha, thanks.
>
>> 
>> >> I expect if anything else mattered we would need to change it to
>> >> HANDLER_EXIT.
>> >> 
>> >> I made a mistake conflating to cases and I want to make certain I
>> >> successfully separate those two cases at the end of the day.
>> >
>> > For skipping the exit work, I'm not sure it matters, since all the
>> > signal stuff is "too late"...
>> 
>> The conflation lead me to believe that we could simply and safely cause
>> seccomp to use normal signal delivery to kill the process.  The first
>> part of the conflation I sorted out by introducing HANDLER_EXIT.  The
>> user visible part of the change I am not yet certain what to do with.
>> 
>> My gut reaction is does it matter?  Can you escape the seccomp filter
>> with a stop?  Does it break userspace?
>
> After fixing UNKILLABLE vs IMMUTABLE, I'm not aware of anything else
> misbehaving. The new nonsense exit event, though, is bound to be at
> least confusing to humans. ("Why did this syscall not change any of its
> registers?", etc.)
>
>> I realize the outcome of that question is that it does matter so we
>> probably need to find a way to supress that situation for HANDLER_EXIT.
>> Both force_exit_sig and force_sig_seccomp appear to be using dumpable
>> signals which makes the problem doubly tricky.
>> 
>> The first tricky bit is fatal_signal_pending isn't set because a
>> coredump is possible, so something else is needed to detect this
>> condition.
>> 
>> The second part is what to do when we detect the condition.
>> 
>> The only solution I can think of quickly is to modify
>> force_sig_info_to_task clear TIF_SYSCALL_WORK on the architectures where
>> that is used and to clear SYSCALL_WORK_EXIT on x86 and s390, and to do
>> whatever the architecture appropriate thing is on the other
>> architectures.
>
> The common accessors for the bits are set_syscall_work()/clear_syscall_work()
> but I don't see anything to operate on an entire mask. Maybe it needs to
> grow something like reset_syscall_work()?

Oh.  I hadn't realized SYSCALL_WORK_EXIT and TIF_SYSCALL_WORK were
masks.  Yes it looks like a simple addition of reset_syscall_work()
and calling it from force_sig_info when HANDLER_EXIT will hide these
events.

When you say the events are corrupted did you mean they return wrong data
to userspace or simply that the events should not fire?

I am trying to figure out if there is a case to be made that it was a
bug that these events were missing.

Eric