Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

On Fri, Feb 7, 2025 at 5:20 PM Eyal Birger [off-list ref] wrote:

On Fri, Feb 7, 2025 at 7:27 AM Jann Horn [off-list ref] wrote:

quoted

On Sun, Feb 2, 2025 at 5:29 PM Eyal Birger [off-list ref] wrote:

quoted

uretprobe(2) is an performance enhancement system call added to improve
uretprobes on x86_64.

Confinement environments such as Docker are not aware of this new system
call and kill confined processes when uretprobes are attached to them.

FYI, you might have similar issues with Syscall User Dispatch
(https://docs.kernel.org/admin-guide/syscall-user-dispatch.html) and
potentially also with ptrace-based sandboxes, depending on what kinda
processes you inject uprobes into. For Syscall User Dispatch, there is
already precedent for a bypass based on instruction pointer (see
syscall_user_dispatch()).

Thanks. This is interesting.

Do you know of confinement environments using this?

Not for Syscall User Dispatch; I think that was mostly intended for
stuff like emulating Windows syscalls in WINE. I'm not sure who
actually uses it, I just know a bit about the kernel side of it.

From what I know, ptrace sandboxing is a technique used by some
configurations of gVisor
(https://gvisor.dev/docs/architecture_guide/platforms/#ptrace), though
now I see that that page says that this configuration is no longer
supported. I am also not sure whether you'd ever have uprobes
installed in files from which instructions are executed in this
environment.

quoted

quoted

Since uretprobe is a "kernel implementation detail" system call which is
not used by userspace application code directly, pass this system call
through seccomp without forcing existing userspace confinement environments
to be changed.

This makes me feel kinda uncomfortable. The purpose of seccomp() is
that you can create a process that is as locked down as you want; you
can use it for some light limits on what a process can do (like in
Docker), or you can use it to make a process that has access to
essentially nothing except read(), write() and exit_group(). Even
stuff like restart_syscall() and rt_sigreturn() is not currently
excepted from that.

Yes, this has been discussed at length in the threads mentioned
in the "Link" tags.

quoted

I guess your usecase is a little special in that you were already
calling from userspace into the kernel with SWBP before, which is also
not subject to seccomp; and the syscall is essentially an
arch-specific hack to make the SWBP a little faster.

Indeed. The uretprobe mechanism wasn't enforced by seccomp before
this syscall. This change preserves this.

quoted

If we do this, we should at least ensure that there is absolutely no
way for anything to happen in sys_uretprobe when no uretprobes are
configured for the process - the first check in the syscall
implementation almost does that, but the implementation could be a bit
stricter. It checks for "regs->ip != trampoline_check_ip()", but if no
uprobe region exists for the process, trampoline_check_ip() returns
`-1 + (uretprobe_syscall_check - uretprobe_trampoline_entry)`. So
there is a userspace instruction pointer near the bottom of the
address space that is allowed to call into the syscall if uretprobes
are not set up. Though the mmap minimum address restrictions will
typically prevent creating mappings there, and
uprobe_handle_trampoline() will SIGILL us if we get that far without a
valid uretprobe.

I'm not sure I understand your point. If creating mappings in that
area is prevented, what is the issue?

It is usually prevented, not always - root can do it depending on
system configuration.

Also, in a syscall like this that will be reachable in every sandbox,
I think we should try to be more careful about edge cases and avoid
things like this offset calculation on address -1.

also, this would be related to the
uretprobe syscall implementation in general, no?

Yes. I just think it is relevant to the seccomp change because
excepting a syscall from seccomp makes it more important that that
syscall is robust and correct.

To me this seems unrelated to the seccomp change.
Jiri, do you have any input on this?

Thanks!
Eyal.