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

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

[KP Singh]

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

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))
 		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
 	 * 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

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

[Andrii Nakryiko]

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.

>                 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
>

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

[Martin KaFai Lau]

On Thu, Jan 07, 2021 at 05:37:29PM +0000, KP Singh 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.
Acked-by: Martin KaFai Lau <kafai@fb.com>

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

[Andrii Nakryiko]

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
> >

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

[KP Singh]

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.

> >
> > >
> > > 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.

All these are good candidates for nullness checks too (I also thought
about bpf_inode and
bpf_task having a null check).

I kind of like the explicit check / input validation in the helper
before it does anything with the pointer.
It's a reminder that the value cannot be assumed to be NULL.

FWIW, we do a similar explicit check in the socket storage code as well.

[...]

> >
> > Gmail highlights a typo in "gurranteed" ;)

Thanks, and thanks gmail ;)

> >
> > >          * 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
> > >

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

[Daniel Borkmann]

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.

Thanks,
Daniel

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

[KP Singh]

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