Fix reporting of SS_ONSTACK

[vangyzen]

Summary

Fix reporting of SS_ONSTACK in nested signal delivery when sigaltstack is used on some architectures.

Test Plan

When I add the bug to amd64, the included kyua test finds it. Without the bug, it passes. I did not run it on other architectures, but I expect it will now pass.

[jhb]

The difference is that the sigonstack() can override ss_flags. sendsig() functions call sigonstack() passing in the current stack pointer. I think this function tries to determine if the current stack pointer is already in the signal stack. If it is, it returns 0 to force a nested signal to use the normal thread stack instead of the signal stack.

However, the sendsig() functions don't use the value of ss_flags in uc_stack, instead they always use the return value of sigonstack() to determine where to place the signal frame. I think the issue though is that if you had a nested signal while using a signal stack, you would the ss_flags would be wrong for the nested signal (it would claim to be on the signal stack when it was actually on the normal thread stack). However, sys_sigreturn() doesn't check ss_flags in uc_stack. On amd64 at least it does check the separate mc_onstack member of 'mcontext_t', but ss_flags in uc_stack is ignored, so I think this bug would be harmless.

You could write a test that installed an alternate signal stack and raised a nested signal from a signal handler and then check the the 'uap->uc_stack.ss_flags' field in the nested signal handler (the nested signal handler has to be installed with SA_SIGINFO). It should fail on these architectures and pass on others.

[kib]

In D18347#389623, @jhb wrote:

The difference is that the sigonstack() can override ss_flags. sendsig() functions call sigonstack() passing in the current stack pointer. I think this function tries to determine if the current stack pointer is already in the signal stack. If it is, it returns 0 to force a nested signal to use the normal thread stack instead of the signal stack.

Are you sure ? From my reading, it returns 1 if TDP_ALTSTACK is set _and_ current %sp is inside the alt stack address range. Well, it would be cleaner if the check was added for sp < ss_sp, otherwise it depends on ss_size comparision with unsigned overflow.

I.e. after the change, ss_flags correctly report SS_ONSTACK if we are on alt stack, 0 if alt stack is configured but we are not on it, and SS_DISABLE is no alt stack. I do not see ss_flags.SS_ONSTACK used anywhere for the in-kernel decisions.

In my opinion, the patch fixes reporting of the ss_flags to userspace, but thats all.

However, the sendsig() functions don't use the value of ss_flags in uc_stack, instead they always use the return value of sigonstack() to determine where to place the signal frame. I think the issue though is that if you had a nested signal while using a signal stack, you would the ss_flags would be wrong for the nested signal (it would claim to be on the signal stack when it was actually on the normal thread stack). However, sys_sigreturn() doesn't check ss_flags in uc_stack. On amd64 at least it does check the separate mc_onstack member of 'mcontext_t', but ss_flags in uc_stack is ignored, so I think this bug would be harmless.

You could write a test that installed an alternate signal stack and raised a nested signal from a signal handler and then check the the 'uap->uc_stack.ss_flags' field in the nested signal handler (the nested signal handler has to be installed with SA_SIGINFO). It should fail on these architectures and pass on others.

[jhb]

In D18347#389943, @kib wrote:

In D18347#389623, @jhb wrote:

The difference is that the sigonstack() can override ss_flags. sendsig() functions call sigonstack() passing in the current stack pointer. I think this function tries to determine if the current stack pointer is already in the signal stack. If it is, it returns 0 to force a nested signal to use the normal thread stack instead of the signal stack.

Are you sure ? From my reading, it returns 1 if TDP_ALTSTACK is set _and_ current %sp is inside the alt stack address range. Well, it would be cleaner if the check was added for sp < ss_sp, otherwise it depends on ss_size comparision with unsigned overflow.

Ah, I I think I just had the return values backwards. Agree that the check depends on unsigned overflow. A comment above sigonstack() explaining its purpose would be nice as well.

I.e. after the change, ss_flags correctly report SS_ONSTACK if we are on alt stack, 0 if alt stack is configured but we are not on it, and SS_DISABLE is no alt stack. I do not see ss_flags.SS_ONSTACK used anywhere for the in-kernel decisions.

In my opinion, the patch fixes reporting of the ss_flags to userspace, but thats all.

Agreed. The kernel seems to use mc_onstack instead in sigreturn.

However, the sendsig() functions don't use the value of ss_flags in uc_stack, instead they always use the return value of sigonstack() to determine where to place the signal frame. I think the issue though is that if you had a nested signal while using a signal stack, you would the ss_flags would be wrong for the nested signal (it would claim to be on the signal stack when it was actually on the normal thread stack). However, sys_sigreturn() doesn't check ss_flags in uc_stack. On amd64 at least it does check the separate mc_onstack member of 'mcontext_t', but ss_flags in uc_stack is ignored, so I think this bug would be harmless.

You could write a test that installed an alternate signal stack and raised a nested signal from a signal handler and then check the the 'uap->uc_stack.ss_flags' field in the nested signal handler (the nested signal handler has to be installed with SA_SIGINFO). It should fail on these architectures and pass on others.

Might still be a good idea to have some tests using sigaltstack() if we don't already.

[kib]

sys/arm/arm/machdep.c

657

While moving the lines around, it is useful to fix more of style.

No need to () around onstack. '?' should go into the previous line. The bit should be tested with != 0.

[vangyzen]

• add sigaltstack SS_ONSTACK test
• fix more style
• MFH

[vangyzen]

tests/sys/kern/sigaltstack.c

45 ↗

(On Diff #51379)

Note to self: I forgot to test this in the body.

[kib]

sys/riscv/riscv/machdep.c

593

I wonder if this expression, or the whole three line, should be moved into a macro/function and used by all arches.