On Thu, Jan 7, 2021 at 11:07 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Thu, Jan 7, 2021 at 9:37 AM KP Singh <kpsingh@xxxxxxxxxx> 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@xxxxxxxxxxxxxx > > > > Fixes: 4cf1bc1f1045 ("bpf: Implement task local storage") > > Fixes: 8ea636848aca ("bpf: Implement bpf_local_storage for inodes") > > Reported-by: Gilad Reti <gilad.reti@xxxxxxxxx> > > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx> > > --- > > 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 > >
