netdev - Re: [PATCH] tracepoint: Add tracepoint_probe_register_may

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]

Date:   Wed, 7 Jul 2021 15:12:28 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        syzbot+721aa903751db87aa244@...kaller.appspotmail.com,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Ingo Molnar <mingo@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        netdev <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH] tracepoint: Add tracepoint_probe_register_may_exist() for
 BPF tracing

On Tue, Jun 29, 2021 at 6:55 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@...dmis.org>
>
> All internal use cases for tracepoint_probe_register() is set to not ever
> be called with the same function and data. If it is, it is considered a
> bug, as that means the accounting of handling tracepoints is corrupted.
> If the function and data for a tracepoint is already registered when
> tracepoint_probe_register() is called, it will call WARN_ON_ONCE() and
> return with EEXISTS.
>
> The BPF system call can end up calling tracepoint_probe_register() with
> the same data, which now means that this can trigger the warning because
> of a user space process. As WARN_ON_ONCE() should not be called because
> user space called a system call with bad data, there needs to be a way to
> register a tracepoint without triggering a warning.
>
> Enter tracepoint_probe_register_may_exist(), which can be called, but will
> not cause a WARN_ON() if the probe already exists. It will still error out
> with EEXIST, which will then be sent to the user space that performed the
> BPF system call.
>
> This keeps the previous testing for issues with other users of the
> tracepoint code, while letting BPF call it with duplicated data and not
> warn about it.

There doesn't seem to be anything conceptually wrong with attaching
the same BPF program twice to the same tracepoint. Is it a hard
requirement to have a unique tp+callback combination, or was it done
mostly to detect an API misuse? How hard is it to support such use
cases?

I was surprised to discover this is not supported (though I never had
a use for this, had to construct a test to see the warning).

>
> Link: https://lore.kernel.org/lkml/20210626135845.4080-1-penguin-kernel@I-love.SAKURA.ne.jp/
> Link: https://syzkaller.appspot.com/bug?id=41f4318cf01762389f4d1c1c459da4f542fe5153 [1]`
>
> Cc: stable@...r.kernel.org
> Fixes: c4f6699dfcb85 ("bpf: introduce BPF_RAW_TRACEPOINT")
> Reported-by: syzbot <syzbot+721aa903751db87aa244@...kaller.appspotmail.com>
> Reported-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> ---
>
> #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>
>  include/linux/tracepoint.h | 10 ++++++++++
>  kernel/trace/bpf_trace.c   |  3 ++-
>  kernel/tracepoint.c        | 33 ++++++++++++++++++++++++++++++---
>  3 files changed, 42 insertions(+), 4 deletions(-)
>

[...]