Re: [PATCH bpf] bpf: local storage helpers should check nullness of owner ptr passed

[KP Singh <kpsingh@kernel.org>]

On Mon, Jan 11, 2021 at 3:07 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 1/7/21 9:25 PM, KP Singh wrote:
> > On Thu, Jan 7, 2021 at 8:15 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >> On Thu, Jan 7, 2021 at 11:07 AM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >>> On Thu, Jan 7, 2021 at 9:37 AM KP Singh <kpsingh@kernel.org> wrote:
> >>>>
> >>>> The verifier allows ARG_PTR_TO_BTF_ID helper arguments to be NULL, so
> >>>> helper implementations need to check this before dereferencing them.
> >>>> This was already fixed for the socket storage helpers but not for task
> >>>> and inode.
> >>>>
> >>>> The issue can be reproduced by attaching an LSM program to
> >>>> inode_rename hook (called when moving files) which tries to get the
> >>>> inode of the new file without checking for its nullness and then trying
> >>>> to move an existing file to a new path:
> >>>>
> >>>>    mv existing_file new_file_does_not_exist
> >>>
> >>> Seems like it's simple to write a selftest for this then?
> >
> > Sure, I will send in a separate patch for selftest and also for the typo.
>
> If it's small or trivial to add a selftest for the fix, I'd suggest to add it
> as part of this series for 'ease of logistics' as it would otherwise be a bit
> odd to i) either have a stand-alone patch against bpf tree with just a selftest
> or ii) having to wait until bpf syncs into bpf-next and then send one against
> bpf-next where for the latter there's risk that it gets forgotten in meantime
> as it might take a while.

Sounds good, I completely missed the fact the fix would take some time to
sync from bpf to bpf-next and until then the selftest will keep crashing.

I updated the selftest and will send in a v2 and while we are at it,
will also fix
the typo Andrii found.

- KP

>
> Thanks,
> Daniel