Re: [PATCH bpf-next v8 4/4] selftests/bpf: Add tests for bpf_rcu_read_lock()

[Martin KaFai Lau <martin.lau@linux.dev>]

On 11/22/22 5:39 PM, Martin KaFai Lau wrote:
> On 11/22/22 5:13 PM, Yonghong Song wrote:
>>
>>
>> On 11/22/22 4:56 PM, Martin KaFai Lau wrote:
>>> On 11/22/22 11:53 AM, Yonghong Song wrote:
>>>> +SEC("?fentry.s/" SYS_PREFIX "sys_nanosleep")
>>>> +int task_acquire(void *ctx)
>>>> +{
>>>> +    struct task_struct *task, *real_parent;
>>>> +
>>>> +    task = bpf_get_current_task_btf();
>>>> +    bpf_rcu_read_lock();
>>>> +    real_parent = task->real_parent;
>>>> +    /* acquire a reference which can be used outside rcu read lock region */
>>>> +    real_parent = bpf_task_acquire(real_parent);
>>> Does the bpf_task_acquire() kfunc need a change to do refcount_inc_not_zero() 
>>> and KF_RET_NULL?
>>
>> We have this definition in kernel:
>> BTF_ID_FLAGS(func, bpf_task_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS)
>>
>> So the argument is trusted args so, either marked as PTR_TRUSTED/MEM_ALLOC or 
>> have a reference acquired already, so
>> I guess we should be fine here.
> 
> 
> The verifier part is fine on {KF_TRUSTED_ARGS, PTR_TRUSTED}.
> 
> iiuc, PTR_TRUSTED means the kfunc can safely dereference the pointer because the 
> ptr has not been freed yet but does not mean its refcnt > 0 and not on its way 
> to be freed after the rcu gp.
> 
> If real_parent's refcnt is 0 here, bpf_task_acquire() will resurrect a task 
> which is on its way to be freed and the task can be stored in a map, so a UAF.


This could be addressed as a follow up though since it is not specific to this set.