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