netdev - Re: [PATCH bpf-next v2 2/2] bpf: disallow BPF_LOG_KERNEL log level for sys(BPF_BTF

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: <30561c6f-eac4-cf65-3eb9-1cd790765071@huawei.com>
Date:   Tue, 2 Nov 2021 12:00:28 +0800
From:   Hou Tao <houtao1@...wei.com>
To:     Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>
CC:     Martin KaFai Lau <kafai@...com>, Yonghong Song <yhs@...com>,
        "Andrii Nakryiko" <andrii@...nel.org>, <netdev@...r.kernel.org>,
        <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 2/2] bpf: disallow BPF_LOG_KERNEL log level
 for sys(BPF_BTF_LOAD)

Hi,

On 11/2/2021 5:59 AM, Daniel Borkmann wrote:
> On 10/29/21 3:53 PM, Hou Tao wrote:
>> BPF_LOG_KERNEL is only used internally, so disallow bpf_btf_load()
>> to set log level as BPF_LOG_KERNEL. The same checking has already
>> been done in bpf_check(), so factor out a helper to check the
>> validity of log attributes and use it in both places.
>>
>> Signed-off-by: Hou Tao <houtao1@...wei.com>
>> ---
>>   include/linux/bpf_verifier.h | 6 ++++++
>>   kernel/bpf/btf.c             | 3 +--
>>   kernel/bpf/verifier.c        | 6 +++---
>>   3 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
>> index c8a78e830fca..b36a0da8d5cf 100644
>> --- a/include/linux/bpf_verifier.h
>> +++ b/include/linux/bpf_verifier.h
>> @@ -396,6 +396,12 @@ static inline bool bpf_verifier_log_needed(const struct
>> bpf_verifier_log *log)
>>            log->level == BPF_LOG_KERNEL);
>>   }
>>   +static inline bool bpf_verifier_log_attr_valid(const struct
>> bpf_verifier_log *log)
>> +{
>> +    return (log->len_total >= 128 && log->len_total <= UINT_MAX >> 2 &&
>> +        log->level && log->ubuf && !(log->level & ~BPF_LOG_MASK));
>
> nit: No surrounding () needed.
Will fix.
>
> This should probably also get a Fixes tag wrt BPF_LOG_KERNEL exposure?
If log->level is set as BPF_LOG_KERNEL, the only harm is the user-space tool
(still need being bpf_capable()) may flood the kernel with BPF error message,
so i didn't add it. Adding the Fixes tags incurs no harm, so will do in v3.
>
> Is there a need to bump log->len_total for BTF so significantly?
>
I had noticed the values of these two max length are different, but doesn't find
any clue about why the different is necessary. So just use the bigger one for
the simplicity of bpf_verifier_log_attr_valid().  Will pass the required max
length to bpf_verifier_log_attr_valid() in v3.