Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering

[Kees Cook]

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?

> [...]
> 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. 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

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

Also please add a KUnit tests to cover this in
tools/testing/selftests/seccomp/seccomp_bpf.c
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)

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().

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

> +	   )
> +		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.

>  	-1, /* negative terminated */
>  };
>  
> -- 
> 2.43.0
> 

-Kees

-- 
Kees Cook