On Thu, Jan 7, 2021 at 8:15 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > 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? 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@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. 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 > > >
