Make invoking `IA2_DEFINE_SIG{ACTION,HANDLER}` inside a function a compile-time error

[kkysen]

When I tried to use IA2_DEFINE_SIGHANDLER and IA2_SIGHANDLER in compartmentalizing dav1d, https://github.com/immunant/dav1d/blob/6483e57832c756096acab2c9f458676027cbb3b0/tools/dav1d.c#L287-L293, there was a segfault in IA2_DEFINE_SIGHANDLER that corrupted the stack. Thus, I worked around this by simply commenting out that code and just didn't install the signal handler, as it's not too important for dav1d. Am I doing something wrong here? I was trying to follow the docs at https://github.com/immunant/IA2-Phase2/blob/94f890bf02fad5bc84c3b0a417fb33b654c7268a/docs/usage.md?plain=1#L81-L86, though they're not too explanatory.

[fw-immunant]

It's surprising to me that this causes a segfault even in situations where (presumably) these signals are not being received. The macros in question define functions in assembly but should not actually cause any change in control flow until the signal handler runs.

I would try debugging stack-mangling crashes like this using rr to reverse execution up to the point where things first go off the rails.

[kkysen]

I would try debugging stack-mangling crashes like this using rr to reverse execution up to the point where things first go off the rails.

Ah, that's a good idea. Let me try rr.

[ayrtonm]

dav1d uses SA_RESETHAND so we should make sure compartment-aware handlers work as expected with that. My understanding of sigaction(2) is that this replaces our compartment-aware handler with the default.

[ayrtonm]

Turns out the issue was that we had IA2_DEFINE_SIGNAL_HANDLER in a function (so the handler it defines was getting embedded in the middle of the function). Aside from noting that this macro can't be used inside a function, we might be able to make this mistake a compiler error by defining handlers as naked functions on x86 (I'm not sure if there is an equivalent tweak on ARM).

[kkysen]

Turns out the issue was that we had IA2_DEFINE_SIGNAL_HANDLER in a function (so the handler it defines was getting embedded in the middle of the function). Aside from noting that this macro can't be used inside a function, we might be able to make this mistake a compiler error by defining handlers as naked functions on x86 (I'm not sure if there is an equivalent tweak on ARM).

Everything works now that IA2_DEFINE_SIGNAL_HANDLER is called in global scope. I'm adding that to the docs, but I looked into if we can detect if the macro is called inside of a function vs. globally, and I don't think we can (things like using __func__, __FUNCTION__, __builtin_FUNCTION(), etc.). However, I'm not understanding why the macro defines a function prototype and then declares the function in asm, vs. defining the function in C with the function body being asm. What's the difference? I'm assuming this was done for some reason, but I'm not sure what it is.

[ayrtonm]

When the kernel makes a thread start executing from the signal handler it sets the PKRU to not have access to anything so it has to be a naked function. Otherwise the first few instructions accessing the stack will fault.

[ayrtonm]

Looks like we didn't initially make them naked functions because the asm statement is used for two functions with different signatures. This isn't a hard blocker for making them naked functions and triggering a compile-time error when these macros are invoked outside global scope, but the ARM case may complicate things. I'm not sure we need a wrapper to set x18 on ARM though. I think the kernel should leave x18 as-is (since it's also used for things like shadow call stack which should be all userspace) so it may be a non-issue.