From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: KP Singh <kpsingh@kernel.org>
Cc: bpf <bpf@vger.kernel.org>, Gilad Reti <gilad.reti@gmail.com>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <kafai@fb.com>
Subject: Re: [PATCH bpf] bpf: local storage helpers should check nullness of owner ptr passed
Date: Thu, 7 Jan 2021 11:07:13 -0800 [thread overview]
Message-ID: <CAEf4BzbxVtR+kaTFyHiH0tz3npr_vnpOidmG=t4sQAtaNE95UA@mail.gmail.com> (raw)
In-Reply-To: <20210107173729.2667975-1-kpsingh@kernel.org>
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
>
next prev parent reply other threads:[~2021-01-07 19:08 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-07 17:37 [PATCH bpf] bpf: local storage helpers should check nullness of owner ptr passed KP Singh
2021-01-07 19:07 ` Andrii Nakryiko [this message]
2021-01-07 19:15 ` Andrii Nakryiko
2021-01-07 20:25 ` KP Singh
2021-01-11 14:06 ` Daniel Borkmann
2021-01-11 17:19 ` KP Singh
2021-01-07 19:11 ` Martin KaFai Lau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEf4BzbxVtR+kaTFyHiH0tz3npr_vnpOidmG=t4sQAtaNE95UA@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=gilad.reti@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).