Pass the syscall number to capsicum permission-denied signals

[theraven]

Summary

The syscall number is stored in the same register as the syscall return on amd64 (and possibly other architectures) and so it is impossible to recover in the signal handler after the call has returned. This small tweak delivers it in the si_value field of the signal, which is sufficient to catch capability violations and emulate them with a call to a more-privileged process in the signal handler.

[kib]

si_value is not the best place for this kind of information. Signal-specific data is supplied in the _reason union of the struct __siginfo. It is cleaner IMO to define a new union member and pass the syscall number through it, preserving the proper use of the interface.

[theraven]

@kib, thank you, that also mirrors what Linux does for seccomp violation signals. I will update the diff to reflect that.

[theraven]

Updated based on @kib's feedback.

I am still somewhat concerned that this doesn't let you differentiate between a syscall made directly and via a syscall / __syscall system call, which will have different argument frame layouts. This information seems to be erased on syscall entry. I'd be happy to make a larger change if you can suggest a good place to stash this information.

[emaste]

As a side comment @theraven if you have to upload a new version next time would you add context via e.g. git show -U99999

[theraven]

@emaste, sorry, I don't think I understand this comment.

[lwhsu]

@theraven that means uploading a full(er) diff, which is easier to check the other content in the modified files. This can also be done through using arcanist tool (devel/arcanist). More information is available at https://wiki.freebsd.org/Phabricator

[theraven]

I appear to be setting the field in the siginfo structure incorrectly in the new version, but it's now the end of the day for me so I will try to debug this tomorrow.

[kib]

In D29185#654121, @theraven wrote:

Updated based on @kib's feedback.

I am still somewhat concerned that this doesn't let you differentiate between a syscall made directly and via a syscall / __syscall system call, which will have different argument frame layouts. This information seems to be erased on syscall entry. I'd be happy to make a larger change if you can suggest a good place to stash this information.

struct syscall_args is the right place, of course. You would need to do a pass over all MD/ABI cpu_fetch_syscall_args() implementations.

When do you need this? Isn't this kind of introspection warrants a use of real debugger which can deduce all the data from debugging information?

[theraven]

When do you need this? Isn't this kind of introspection warrants a use of real debugger which can deduce all the data from debugging information?

We are using this to implement the FreeBSD version of something we have working with seccomp-bpf already: trapping when a disallowed syscall occurs and turning it into an RPC to a more privileged process. We don't have a debugger attached. The FreeBSD decision to use SIGTRAP is somewhat unfortunate because debuggers don't deliver this to the traced process if they see it and so we can't debug code using this on FreeBSD (Linux uses SIGSYS and so we can).

[theraven]

Add amd64 paths to preserve the original system call number.

If this is the right approach then I will make the same changes for other
architectures.

[theraven]

Sorry, this version does work, I had only updated one of the places in the code using it to check si_syscall and so the test was failing yesterday.

[kib]

This looks good to me.

sys/sys/signal.h

259 ↗

(On Diff #85633)

Look how other comments are formatted. Add (at least one) tab after ';' and possibly wrap line if longer than 80 chars.

[emaste]

The FreeBSD decision to use SIGTRAP is somewhat unfortunate because debuggers don't deliver this to the traced process if they see it

SIGTRAP on capability errno was intended as a debugging aid, I did not anticipate it would be used in this manner. I'm not sure what signal might be more appropriate for this case though.

[theraven]

• Add missing architecture support, address reviewer comments.

[theraven]

Are the syscall_args structures part of the stable KBI? This won't quite cleanly apply to older versions of FreeBSD because at least on x86 the system call entry code has been cleaned up to remove some code duplication, but I am running on 12-STABLE with a version of this that doesn't correctly handle the syscall / __syscall syscalls and it would be great to be able to MFC as much as possible, even if we can't get full support without the KBI change.

[kib]

In D29185#655872, @theraven wrote:

Are the syscall_args structures part of the stable KBI? This won't quite cleanly apply to older versions of FreeBSD because at least on x86 the system call entry code has been cleaned up to remove some code duplication, but I am running on 12-STABLE with a version of this that doesn't correctly handle the syscall / __syscall syscalls and it would be great to be able to MFC as much as possible, even if we can't get full support without the KBI change.

By themself, syscall_args are not used by anything but syscall entry code (and some ptrace bits). But the structure is embedded into struct thread, and layout of struct thread must be stable.

What do you mean by saying that some stable/12 does not handle the syscall syscall correctly? Against which version of FreeBSD is your patch generated?

[kib]

sys/amd64/include/proc.h

95 ↗

(On Diff #85838)

Note that this addition of u_int does not change layout or size of syscall_args because there is a gap after code.

sys/i386/include/proc.h

66 ↗

(On Diff #85838)

But this addition changes layout and probably triggers compile-time asserts in kern_thread.c.

BTW you are inconsistent: amd64 added original_code after code, i386 is reversed.

[theraven]

In D29185#655877, @kib wrote:

In D29185#655872, @theraven wrote:

By themself, syscall_args are not used by anything but syscall entry code (and some ptrace bits). But the structure is embedded into struct thread, and layout of struct thread must be stable.

I see. So it would be possible to MFC the changes to the layouts on 64-bit platforms if original_code is after code and therefore inside padding, but not the changes in 32-bit architectures? Should I split them into two separate patches?

What do you mean by saying that some stable/12 does not handle the syscall syscall correctly? Against which version of FreeBSD is your patch generated?

The patch is against main. I also have a version done against stable/12, but to support the syscall syscall you need the original_code bits, which change the structure, and so I wanted to understand what is possible to MFC.

[theraven]

sys/i386/include/proc.h

66 ↗

(On Diff #85838)

It's before in both, but from what you say about KBIs it should be after on 64-bit architectures to enable MFC and it doesn't matter (but for consistency should probably be after) on 32-bit ones.

[kib]

For stable/12 and stable/13, you can add a new member to struct thread at the end. Then you need to add some manual code to copy this member on fork.

[bcr]

OK from manpages.

[theraven]

• Reorder original_code to fit in padding for 64-bit archs.

[theraven]

Thanks. I think I'm happy for this version to land in main if reviewers are and I'll work on a version for 12- and 13-stable.

[theraven]

12-STABLE version is D30635.

[kib]

sys/sys/signal.h

261 ↗

(On Diff #90396)

'blocked' is used there as 'denied' (or whatever more appropriate English word). By 'blocked' we typically mean 'put off CPU for scheduling purposes due to resource unavailability'.

[theraven]

• Use less-ambiguous terminology.

[theraven]

Thanks. Please can you commit if you're happy with it?

[kib]

In D29185#688188, @theraven wrote:

Thanks. Please can you commit if you're happy with it?

Yes. Please do something like git send-email with the proper fully-formatted commit message and (most important) Author filled. I think you still can use [e-mail] but this is your call.

[theraven]

I've never used git send-mail, how does it interact with Phabricator? If I do a rebase / squash to produce a single commit, is that sufficient? You should be able to then arc patch and git push.

Or I suppose could ping core and get my commit bit reactivated, since I seem to have a bit of time to do FreeBSD things again...

[emaste]

In D29185#688215, @theraven wrote:

I've never used git send-mail, how does it interact with Phabricator? If I do a rebase / squash to produce a single commit, is that sufficient? You should be able to then arc patch and git push.

Or I suppose could ping core and get my commit bit reactivated, since I seem to have a bit of time to do FreeBSD things again...

The 2nd option seems simple enough to me but I might have an ulterior motive :)

You can squash to a single commit, and git format-patch HEAD^ to get a patch file with author metadata & commit message. I believe author data (at a minimum) gets lost with a trip through arc/phab.

[theraven]

Let's try option 2 and see if it works :-)

[jhb]

Please document the new field in siginfo(3).

[theraven]

• Add missing siginfo(3) documentation.