> On Jun 1, 2023, at 9:27 AM, KP Singh <kpsingh@xxxxxxxxxx> wrote:
>
> On Thu, Jun 1, 2023 at 6:18 PM Song Liu <song@xxxxxxxxxx> wrote:
>>
>> On Thu, Jun 1, 2023 at 5:26 AM KP Singh <kpsingh@xxxxxxxxxx> wrote:
>>>
>>> When the the task local storage was generalized for tracing programs, the
>>> bpf_task_local_storage callback was moved from a BPF LSM hook callback
>>> for security_task_free LSM hook to it's own callback. But a failure case
>>> in bad_fork_cleanup_security was missed which, when triggered, led to a dangling
>>> task owner pointer and a subsequent use-after-free.
>>>
>>> This issue was noticed when a BPF LSM program was attached to the
>>> task_alloc hook on a kernel with KASAN enabled. The program used
>>> bpf_task_storage_get to copy the task local storage from the current
>>> task to the new task being created.
>>
>> This is pretty tricky. Let's add a selftest for this.
>
> I don't have an easy repro for this (the UAF does not trigger
> immediately), Also I am not sure how one would test a UAF in a
> selftest. What actually happens is:
>
> * We have a dangling task pointer in local storage.
> * This is used sometime later which then leads to weird memory
> corruption errors.
I think we will see it easily with KASAN, no?
>
>>
>>>
>>> Fixes: a10787e6d58c ("bpf: Enable task local storage for tracing programs")
>>> Reported-by: Kuba Piecuch <jpiecuch@xxxxxxxxxx>
>>> Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
>>> ---
>>>
>>> This fixes the regression from the LSM blob based implementation, we can
>>> still have UAFs, if bpf_task_storage_get is invoked after bpf_task_storage_free
>>> in the cleanup path.
>>
>> Can we fix this by calling bpf_task_storage_free() from free_task()?
>
> I think we can yeah. But, this is yet another deviation from how the
> security blob is managed (security_task_free) frees the blob that we
> were previously using.
Yeah, this will make the code even more tricky.
Another idea I had is to filter on task->__state in the helper. IOW,
task local storage does not work on starting or died tasks. But I am
not sure whether this will make BPF_LSM less effective (not covering
certain tasks).
Thanks,
Song
