BUG: SCMP_ACT_TRAP should receive a `data`

[RocketMaDev]

Problem Reveal

Recently I examined seccomp filter in chromium and found it had a value in act TRAP.

I'm using a cooperated seccomp tool called ceccomp with ceccomp trace --pid=4187 to dump the filter in browsers, from which I take a screenshot. In the screenshot, line 604 ends with 0x00030001, where the last 1 is data. To confirm that truly works, I checked out manual:

SECCOMP_RET_TRAP This value results in the kernel sending a thread-directed SIGSYS signal to the triggering thread. (The system call is not executed.) Various fields will be set in the siginfo_t structure (see sigaction(2)) associated with signal:

• si_signo will contain SIGSYS.

• si_call_addr will show the address of the system call instruction.

• si_syscall and si_arch will indicate which system call was attempted.

• si_code will contain SYS_SECCOMP.

• si_errno will contain the SECCOMP_RET_DATA portion of the filter return value.

The program counter will be as though the system call happened (i.e., the program counter will not point to the system call instruction). The return value register will contain an architecture-dependent value; if resuming execution, set it to something appropriate for the system call. (The architecture dependency is because replacing it with ENOSYS could overwrite some useful information.)

I also read kernel source code and found the data truly works. Tracking back kernel versions, the kernel had enabled this feature since v3.

Bug Found

Now talk back about libseccomp, it exposes an API to allow users to determine what action when performing the filter. SCMP_ACT_ERRNO and SCMP_ACT_TRACE are two macros which accept a uint16_t data, while SCMP_ACT_TRAP is only a constant.

According to exploration above, SCMP_ACT_TRAP should also take data like SCMP_ACT_ERRNO or SCMP_ACT_TRACE.

Two Choices

This is definitely a flaw and need to be fixed. However, should we make a breaking change?

• Refactor SCMP_ACT_TRAP to SCMP_ACT_TRAP(x), which keeps behavior like errno or trace, but may break downstream softwares.
• Add a new macro like SCMP_ACT_TRAPX(x) to accept the value, which have no impact on compatibility, but may be confusing for users to have a similar SCMP_ACT_TRAP.

I would like to make contributions and this is my main concern. Which proposal will you suggest?

[hurricane618]

I support the first refactoring plan, which will be beneficial for subsequent code maintenance.

[RocketMaDev]

@pcmoore Waiting to hear your decision

[drakenclimber]

Interesting find, @RocketMaDev. Thanks for the thorough bug report.

We'll have to discuss how to handle this, but I can answer a few questions:

• libseccomp v2.5.z - we've likely released the last release of libseccomp v2.5, so this won't be changed there.
• libseccomp v2.6.z - When we release a vX.Y.Z version of libseccomp, we guarantee API and ABI compatibility for the life of that vX.Y.Z release. So libseccomp v2.6.z will definitely not see a change of the definition of SCMP_ACT_TRAP. Also, we are hesitant to add new features to already released versions of libseccomp, and we try to release new versions when we roll the MINOR (and sometimes MAJOR) version numbers. tl;dr - this won't go in v2.6.z.

A change to support data in SCMP_ACT_TRAP will have to be in a future release - either v2.7 or v3.0 (if we end up breaking the API). I can see arguments for either SCMP_ACT_TRAP(x) or SCMP_ACT_TRAPX(x), so I'll need to think some more about this. There are pros/cons to both solutions.

[drakenclimber]

FYI, @RocketMaDev, I have added a pull request to potentially address this issue. I would love to hear your thoughts. Thanks :)

[RocketMaDev]

@drakenclimber The PR looks awesome! :heart: