On Wed, Jan 6, 2021 at 10:02 PM KP Singh <kpsingh@xxxxxxxxxx> wrote: > > On Wed, Jan 6, 2021 at 6:27 PM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Wed, Jan 6, 2021 at 2:18 AM Gilad Reti <gilad.reti@xxxxxxxxx> wrote: > > > > > > Hello all, > > > > > > Hope you had a great new year vacation. > > > > > > Sorry for bumping up the thread, but I believe that it is an important one. > > > > Thanks for bumping. > > KP, please take a look. > > > > > On Sun, Dec 20, 2020 at 12:16 PM Gilad Reti <gilad.reti@xxxxxxxxx> wrote: > > > > > > > > Hello there, > > > > > > > > During experimenting with the new inode local storage I have stumbled > > > > across an invalid pointer dereference that passed through the > > > > verifier. > > > > > > > > After investigating, I think that it happens in the > > > > bpf_inode_storage_get helper, and in particular in the following line: > > > > https://elixir.bootlin.com/linux/v5.10.1/source/include/linux/bpf_lsm.h#L32 > > > > > > > > I have a single bpf lsm probe in the security_inode_rename probe, and > > > > I have called bpf_inode_storage_get on the inode of the "new_dentry" > > > > argument. This inode may be null in cases where renameat(2) is called > > > > to move a file from one path to a new path which didn't exist before. > > > > > > > > As can be seen, inode is dereferenced without first checking that it > > > > is not NULL. > > > > I don't know what should be the correct behavior, but I believe that > > > > either the helper should check the validity of passed pointers, or the > > > > verifier should treat fields of BTF pointers (PTR_TO_BTF_ID) as > > > > PTR_TO_BTF_ID_OR_NULL. > > Thanks for adding me, I missed responding to this over the winter holidays. > > In this case this should indeed be PTR_TO_BTF_ID_OR_NULL. I will send in a Okay so I dug in a little more...We don't really need this "PTR_TO_BTF_ID points to a kernel struct that does not need to be null checked by the BPF program. This does not imply the pointer is _not_ null and in practice this can easily be a null pointer when reading pointer chains. The assumption is program context will handle null pointer dereference typically via fault handling. The verifier must keep this in mind and can make no assumptions about null or non-null when doing branch analysis. Further, when passed into helpers the helpers can not, without additional context, assume the value is non-null." So all we need to do is to check the null-ness of the pointer in the helper code, the helpers cannot simply assume that the pointer will be non NULL. This does the trick, I will send in a patch tomorrow: diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c index 6edff97ad594..92a59b283316 100644 --- a/kernel/bpf/bpf_inode_storage.c +++ b/kernel/bpf/bpf_inode_storage.c @@ -168,6 +168,9 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode, { struct bpf_local_storage_data *sdata; + if (!inode) + return (unsigned long)NULL; + if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE)) return (unsigned long)NULL; @@ -200,6 +203,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 * to have a refcount and cannot be freed. */ - KP > fix. > > > > > > > > > I am attaching a minimal program example, along with the kernel demsg > > > > output. To reproduce, load the probe and run "mv file1 file2" where > > > > file2 does not exist. > > Thanks for sending the repro program as well! Really appreciate it! > > > > > > > > > Thanks, > > > > Gilad Reti > > > > > > > > inode_oops.c: > > > > > > > > // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) > > [...] > > > > > [Thu Dec 17 11:35:37 2020] FS: 00007fa878948740(0000) > > > > GS:ffff895cede00000(0000) knlGS:0000000000000000 > > > > [Thu Dec 17 11:35:37 2020] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > > > [Thu Dec 17 11:35:37 2020] CR2: 0000000000000038 CR3: 0000000121f34001 > > > > CR4: 00000000003706f0
