Message-ID: <CAHsH6Gsv3DB0O5oiEDsf2+Go4O1+tnKm-Ab0QPyohKSaroSxxA@mail.gmail.com>
Date: Wed, 29 Jan 2025 09:27:49 -0800
From: Eyal Birger <eyal.birger@...il.com>
To: Kees Cook <kees@...nel.org>
Cc: luto@...capital.net, wad@...omium.org, oleg@...hat.com, 
	mhiramat@...nel.org, andrii@...nel.org, jolsa@...nel.org, 
	alexei.starovoitov@...il.com, olsajiri@...il.com, cyphar@...har.com, 
	songliubraving@...com, yhs@...com, john.fastabend@...il.com, 
	peterz@...radead.org, tglx@...utronix.de, bp@...en8.de, daniel@...earbox.net, 
	ast@...nel.org, andrii.nakryiko@...il.com, rostedt@...dmis.org, rafi@....io, 
	shmulik.ladkani@...il.com, bpf@...r.kernel.org, linux-api@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, x86@...nel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering

Hi,

Thanks for the review!

On Tue, Jan 28, 2025 at 5:41 PM Kees Cook <kees@...nel.org> wrote:
>
> On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> > Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> > uses the same number as __NR_uretprobe so the syscall isn't forced in the
> > compat bitmap.
>
> So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
> uretprobe strictly an x86_64 feature?
>

My understanding is that they'd be able to do so, but use the int3 trap
instead of the uretprobe syscall.

> > [...]
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 385d48293a5f..23b594a68bc0 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> >
> >  #ifdef SECCOMP_ARCH_NATIVE
> >  /**
> > - * seccomp_is_const_allow - check if filter is constant allow with given data
> > + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> >   * @fprog: The BPF programs
> >   * @sd: The seccomp data to check against, only syscall number and arch
> >   *      number are considered constant.
> >   */
> > -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > -                                struct seccomp_data *sd)
> > +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> > +                                       struct seccomp_data *sd)
> >  {
> >       unsigned int reg_value = 0;
> >       unsigned int pc;
> > @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> >       return false;
> >  }
> >
> > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > +                                struct seccomp_data *sd)
> > +{
> > +#ifdef __NR_uretprobe
> > +     if (sd->nr == __NR_uretprobe
> > +#ifdef SECCOMP_ARCH_COMPAT
> > +         && sd->arch != SECCOMP_ARCH_COMPAT
> > +#endif
>
> I don't like this because it's not future-proof enough. __NR_uretprobe
> may collide with other syscalls at some point.

I'm not sure I got this point.

> And if __NR_uretprobe_32
> is ever implemented, the seccomp logic will be missing. I think this
> will work now and in the future:
>
> #ifdef __NR_uretprobe
> # ifdef SECCOMP_ARCH_COMPAT
>         if (sd->arch == SECCOMP_ARCH_COMPAT) {
> #  ifdef __NR_uretprobe_32
>                 if (sd->nr == __NR_uretprobe_32)
>                         return true;
> #  endif
>         } else
> # endif
>         if (sd->nr == __NR_uretprobe)
>                 return true;
> #endif

I don't know if implementing uretprobe syscall for compat binaries is
planned or makes sense - I'd appreciate Jiri's and others opinion on that.
That said, I don't mind adding this code for the sake of future proofing.

>
> Instead of doing a function rename dance, I think you can just stick
> the above into seccomp_is_const_allow() after the WARN().

My motivation for the renaming dance was that you mentioned we might add
new syscalls to this as well, so I wanted to avoid cluttering the existing
function which seems to be well defined.

>
> Also please add a KUnit tests to cover this in
> tools/testing/selftests/seccomp/seccomp_bpf.c

I think this would mean that this test suite would need to run as
privileged. Is that Ok? or maybe it'd be better to have a new suite?

> With at least these cases combinations below. Check each of:
>
>         - not using uretprobe passes
>         - using uretprobe passes (and validates that uretprobe did work)
>
> in each of the following conditions:
>
>         - default-allow filter
>         - default-block filter
>         - filter explicitly blocking __NR_uretprobe and nothing else
>         - filter explicitly allowing __NR_uretprobe (and only other
>           required syscalls)

Ok.

>
> Hm, is uretprobe expected to work on mips? Because if so, you'll need to
> do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
> version of seccomp_cache_check_allow().

I don't know if uretprobe syscall is expected to run on mips. Personally
I'd avoid adding this dead code.

>
> (You can see why I really dislike having policy baked into seccomp!)

I definitely understand :)

>
> > +        )
> > +             return true;
> > +#endif
> > +
> > +     return seccomp_is_filter_const_allow(fprog, sd);
> > +}
> > +
> >  static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
> >                                        void *bitmap, const void *bitmap_prev,
> >                                        size_t bitmap_size, int arch)
> > @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> >   */
> >  static const int mode1_syscalls[] = {
> >       __NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> > +#ifdef __NR_uretprobe
> > +     __NR_uretprobe,
> > +#endif
>
> It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even
> though it doesn't exist. (Is it _never_ planned to be implemented?) But
> then, maybe the chances of a compat mode1 seccomp process running under
> uretprobe is vanishingly small.

It seems to me very unlikely. BTW, when I tested the "strict" mode change
my program was killed by seccomp. The reason wasn't the uretprobe syscall
(which I added to the list), it was actually the exit_group syscall which
libc uses instead of the exit syscall.

Thanks again,
Eyal.

Powered by blists - more mailing lists