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

From: Martin KaFai Lau <martin.lau@linux.dev>
To: Yonghong Song <yhs@meta.com>
Cc: Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	kernel-team@fb.com, Martin KaFai Lau <martin.lau@kernel.org>,
	bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next v8 4/4] selftests/bpf: Add tests for bpf_rcu_read_lock()
Date: Tue, 22 Nov 2022 17:52:59 -0800	[thread overview]
Message-ID: <767c6ae6-4aad-69a1-53e2-b7f8643a79c1@linux.dev> (raw)
In-Reply-To: <ac70f574-4023-664e-b711-e0d3b18117fd@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.