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

[Hou Tao <houtao1@huawei.com>]

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@huawei.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.