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: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.