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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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?
>
> >
> > The report including the sample program and the steps for reproducing
> > the bug:
> >
> >   https://lore.kernel.org/bpf/CANaYP3HWkH91SN=wTNO9FL_2ztHfqcXKX38SSE-JJ2voh+vssw@mail.gmail.com
> >
> > Fixes: 4cf1bc1f1045 ("bpf: Implement task local storage")
> > Fixes: 8ea636848aca ("bpf: Implement bpf_local_storage for inodes")
> > Reported-by: Gilad Reti <gilad.reti@gmail.com>
> > Signed-off-by: KP Singh <kpsingh@kernel.org>
> > ---
> >  kernel/bpf/bpf_inode_storage.c | 5 ++++-
> >  kernel/bpf/bpf_task_storage.c  | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> > index 6edff97ad594..dbc1dbdd2cbf 100644
> > --- a/kernel/bpf/bpf_inode_storage.c
> > +++ b/kernel/bpf/bpf_inode_storage.c
> > @@ -176,7 +176,7 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
> >          * bpf_local_storage_update expects the owner to have a
> >          * valid storage pointer.
> >          */
> > -       if (!inode_storage_ptr(inode))
> > +       if (!inode || !inode_storage_ptr(inode))
>
> would it be bad to move !inode check inside inode_storage_ptr itself?
> same for task_storage_ptr() below.

And for deletes, inode_storage_delete calls into
inode_storage_lookup(), which also seems like a reasonable place to
check for null? Even better, inode_storage_lookup() shares logic with
inode_storage_ptr(), so if we make sure that all code calls
inode_storage_ptr(), then we need to check for NULL just in
inode_storage_ptr().

I totally might be missing some subtleties, of course.

>
> >                 return (unsigned long)NULL;
> >
> >         sdata = inode_storage_lookup(inode, map, true);
> > @@ -200,6 +200,9 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
> >  BPF_CALL_2(bpf_inode_storage_delete,
> >            struct bpf_map *, map, struct inode *, inode)
> >  {
> > +       if (!inode)
> > +               return -EINVAL;
> > +
> >         /* This helper must only called from where the inode is gurranteed
>
> Gmail highlights a typo in "gurranteed" ;)
>
> >          * to have a refcount and cannot be freed.
> >          */
> > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> > index 4ef1959a78f2..e0da0258b732 100644
> > --- a/kernel/bpf/bpf_task_storage.c
> > +++ b/kernel/bpf/bpf_task_storage.c
> > @@ -218,7 +218,7 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *,
> >          * bpf_local_storage_update expects the owner to have a
> >          * valid storage pointer.
> >          */
> > -       if (!task_storage_ptr(task))
> > +       if (!task || !task_storage_ptr(task))
> >                 return (unsigned long)NULL;
> >
> >         sdata = task_storage_lookup(task, map, true);
> > @@ -243,6 +243,9 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *,
> >  BPF_CALL_2(bpf_task_storage_delete, struct bpf_map *, map, struct task_struct *,
> >            task)
> >  {
> > +       if (!task)
> > +               return -EINVAL;
> > +
> >         /* This helper must only be called from places where the lifetime of the task
> >          * is guaranteed. Either by being refcounted or by being protected
> >          * by an RCU read-side critical section.
> > --
> > 2.30.0.284.gd98b1dd5eaa7-goog
> >