Re: [RFC PATCH bpf-next 1/2] bpf: Add bpf_current_capable kfunc

[Yonghong Song <yonghong.song@linux.dev>]

On 8/14/23 10:49 PM, Yafang Shao wrote:
> On Tue, Aug 15, 2023 at 11:40 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>>
>> On 8/14/23 7:45 PM, Yafang Shao wrote:
>>> On Tue, Aug 15, 2023 at 8:28 AM Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>
>>>>
>>>>
>>>> On 8/14/23 7:33 AM, Yafang Shao wrote:
>>>>> Add a new bpf_current_capable kfunc to check whether the current task
>>>>> has a specific capability. In our use case, we will use it in a lsm bpf
>>>>> program to help identify if the user operation is permitted.
>>>>>
>>>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>>>> ---
>>>>>     kernel/bpf/helpers.c | 6 ++++++
>>>>>     1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
>>>>> index eb91cae..bbee7ea 100644
>>>>> --- a/kernel/bpf/helpers.c
>>>>> +++ b/kernel/bpf/helpers.c
>>>>> @@ -2429,6 +2429,11 @@ __bpf_kfunc void bpf_rcu_read_unlock(void)
>>>>>         rcu_read_unlock();
>>>>>     }
>>>>>
>>>>> +__bpf_kfunc bool bpf_current_capable(int cap)
>>>>> +{
>>>>> +     return has_capability(current, cap);
>>>>> +}
>>>>
>>>> Since you are testing against 'current' capabilities, I assume
>>>> that the context should be process. Otherwise, you are testing
>>>> against random task which does not make much sense.
>>>
>>> It is in the process context.
>>>
>>>>
>>>> Since you are testing against 'current' cap, and if the capability
>>>> for that task is stable, you do not need this kfunc.
>>>> You can test cap in user space and pass it into the bpf program.
>>>>
>>>> But if the cap for your process may change in the middle of
>>>> run, then you indeed need bpf prog to test capability in real time.
>>>> Is this your use case and could you describe in in more detail?
>>>
>>> After we convert the capability of our networking bpf program from
>>> CAP_SYS_ADMIN to CAP_BPF+CAP_NET_ADMIN to enhance the security, we
>>> encountered the "pointer comparison prohibited" error, because
>>
>> Could you give a reproducible test case for this verifier failure
>> with CAP_BPF+CAP_NET_ADMIN capability? Is this due to packet pointer
>> or something else? Maybe verifier needs to be improved in these
>> cases without violating the promise of allow_ptr_leaks?
> 
> Here it is.
> 
> SEC("cls-ingress")
> int ingress(struct __sk_buff *skb)
> {
>          struct iphdr *iph = (void *)(long)skb->data + sizeof(struct ethhdr);
> 
>          if ((long)(iph + 1) > (long)skb->data_end)
>                  return TC_ACT_STOLEN;
>          return TC_ACT_OK;
> }
> 
> In this bpf prog, it will compare the pointer iph with skb->data_end,
> and thus it will fail the verifier.

Thanks. In this particular case, I think comparing packet ptr and 
data_end should not be considered as ptr_leaks. Probably Alexei
and Daniel can comment on this too.