linux-kernel - Re: [PATCH bpf-next v2 2/2] bpf: Refactor bpf_tracing_func_proto() and remove bpf_get_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] [day] [month] [year] [list]

Message-ID: <2221d8d3-353e-4403-8675-fadc323b5885@iogearbox.net>
Date: Wed, 27 Nov 2024 13:11:21 +0100
From: Daniel Borkmann <daniel@...earbox.net>
To: Marco Elver <elver@...gle.com>, Alexei Starovoitov <ast@...nel.org>,
 Andrii Nakryiko <andrii@...nel.org>
Cc: 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>, Nikola Grcevski <nikola.grcevski@...fana.com>,
 bpf@...r.kernel.org, linux-trace-kernel@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 2/2] bpf: Refactor bpf_tracing_func_proto()
 and remove bpf_get_probe_write_proto()

On 11/27/24 1:06 PM, Marco Elver wrote:
> On Wed, 27 Nov 2024 at 12:10, Marco Elver <elver@...gle.com> wrote:
>>
>> With bpf_get_probe_write_proto() no longer printing a message, we can
>> avoid it being a special case with its own permission check.
>>
>> Refactor bpf_tracing_func_proto() similar to bpf_base_func_proto() to
>> have a section conditional on bpf_token_capable(CAP_SYS_ADMIN), where
>> the proto for bpf_probe_write_user() is returned. Finally, remove the
>> unnecessary bpf_get_probe_write_proto().
>>
>> This simplifies the code, and adding additional CAP_SYS_ADMIN-only
>> helpers in future avoids duplicating the same CAP_SYS_ADMIN check.
>>
>> Suggested-by: Andrii Nakryiko <andrii@...nel.org>
>> Signed-off-by: Marco Elver <elver@...gle.com>
>> ---
>> v2:
>> * New patch.
>> ---
>>   kernel/trace/bpf_trace.c | 30 ++++++++++++++++++------------
>>   1 file changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>> index 0ab56af2e298..d312b77993dc 100644
>> --- a/kernel/trace/bpf_trace.c
>> +++ b/kernel/trace/bpf_trace.c
>> @@ -357,14 +357,6 @@ static const struct bpf_func_proto bpf_probe_write_user_proto = {
>>          .arg3_type      = ARG_CONST_SIZE,
>>   };
>>
>> -static const struct bpf_func_proto *bpf_get_probe_write_proto(void)
>> -{
>> -       if (!capable(CAP_SYS_ADMIN))
>> -               return NULL;
>> -
>> -       return &bpf_probe_write_user_proto;
>> -}
>> -
>>   #define MAX_TRACE_PRINTK_VARARGS       3
>>   #define BPF_TRACE_PRINTK_SIZE          1024
>>
>> @@ -1417,6 +1409,12 @@ late_initcall(bpf_key_sig_kfuncs_init);
>>   static const struct bpf_func_proto *
>>   bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>>   {
>> +       const struct bpf_func_proto *func_proto;
>> +
>> +       func_proto = bpf_base_func_proto(func_id, prog);
>> +       if (func_proto)
>> +               return func_proto;
> 
> As indicated by the patch robot failure, we can't move this call up
> and needs to remain the last call after all others because we may
> override a function proto in bpf_base_func_proto here (like done for
> BPF_FUNC_get_smp_processor_id).
> 
> Let me fix that.

I was about to comment on that, I would leave this as it was before,
otherwise rest lgtm.