linux-kernel - Re: [PATCH bpf-next] bpf: Improve bpf_probe_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4Bzb4D_=zuJrg3PawMOW3KqF8JvJm9SwF81_XHR2+u5hkUg@mail.gmail.com>
Date: Tue, 26 Nov 2024 13:32:33 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Marco Elver <elver@...gle.com>
Cc: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, 
	Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, bpf@...r.kernel.org, linux-trace-kernel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Nikola Grcevski <nikola.grcevski@...fana.com>
Subject: Re: [PATCH bpf-next] bpf: Improve bpf_probe_write_user() warning message

On Tue, Nov 26, 2024 at 8:54 AM Marco Elver <elver@...gle.com> wrote:
>
> The warning message for bpf_probe_write_user() was introduced in
> 96ae52279594 ("bpf: Add bpf_probe_write_user BPF helper to be called in
> tracers"), with the following in the commit message:
>
>     Given this feature is meant for experiments, and it has a risk of
>     crashing the system, and running programs, we print a warning on
>     when a proglet that attempts to use this helper is installed,
>     along with the pid and process name.
>
> After 8 years since 96ae52279594, bpf_probe_write_user() has found
> successful applications beyond experiments [1, 2], with no other good
> alternatives. Despite its intended purpose for "experiments", that
> doesn't stop Hyrum's law, and there are likely many more users depending
> on this helper: "[..] it does not matter what you promise [..] all
> observable behaviors of your system will be depended on by somebody."
>
> As such, the warning message can be improved:
>
> 1. The ominous "helper that may corrupt user memory!" offers no real
>    benefit, and has been found to lead to confusion where the system
>    administrator is loading programs with valid use cases.  Remove it.
>    No information is lost, and administrators who know their system
>    should not load eBPF programs that use bpf_probe_write_user() know
>    what they are looking for.
>
> 2. If multiple programs with bpf_probe_write_user() are loaded by the
>    same task/PID consecutively, only print the message once. If another
>    task loads a program with the helper, the message is printed once
>    more, and so on. This also makes the need for rate limiting
>    redundant.
>
> 3. Every printk line needs to be concluded with "\n" to be flushed. With
>    the old version the warning message only appeared after any following
>    printk. Fix this.
>
> Link: https://lore.kernel.org/lkml/20240404190146.1898103-1-elver@google.com/ [1]
> Link: https://lore.kernel.org/r/lkml/CAAn3qOUMD81-vxLLfep0H6rRd74ho2VaekdL4HjKq+Y1t9KdXQ@mail.gmail.com/ [2]
> Signed-off-by: Marco Elver <elver@...gle.com>
> ---
>  kernel/trace/bpf_trace.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 630b763e5240..0ead3d66f8db 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -359,11 +359,16 @@ static const struct bpf_func_proto bpf_probe_write_user_proto = {
>
>  static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
>  {
> +       static pid_t last_warn_pid = -1;
> +
>         if (!capable(CAP_SYS_ADMIN))
>                 return NULL;
>
> -       pr_warn_ratelimited("%s[%d] is installing a program with bpf_probe_write_user helper that may corrupt user memory!",
> -                           current->comm, task_pid_nr(current));
> +       if (READ_ONCE(last_warn_pid) != task_pid_nr(current)) {
> +               pr_warn("%s[%d] is installing a program with bpf_probe_write_user\n",
> +                       current->comm, task_pid_nr(current));
> +               WRITE_ONCE(last_warn_pid, task_pid_nr(current));
> +       }

should we just drop this warning altogether? After all, we can call
crash_kexec() without any warnings, if we have the right capabilities.
bpf_probe_write_user() is much less destructive and at worst will
cause memory corruption within a single process (assuming
CAP_SYS_ADMIN, of course). If yes, I think we should drop
bpf_get_probe_write_proto() function altogether and refactor
bpf_tracing_func_proto() to have
bpf_token_capable(CAP_SYS_ADMIN)-guarded section, just like
bpf_base_func_proto() has.

>
>         return &bpf_probe_write_user_proto;
>  }
> --
> 2.47.0.338.g60cca15819-goog
>