On Thu, Jun 1, 2023 at 9:54 AM Song Liu <songliubraving@xxxxxxxx> wrote:
>
>
>
> > 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:
> >>>
<...>
> >>> 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).
>
I thought about this as well. But I think some use cases would want to
use task local storage at task creation. At least attaching at the
sched_newtask tracepoints should be fine in those cases. But at the
time of sched_newtask, task->__state is still TASK_NEW.
Another idea, is it possible to allow LSM + local storage on
starting/dying tasks, but restrict tracing + local storage to created
tasks?
Hao
