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