On Tue, Feb 22, 2022 at 02:15:37AM IST, Alexei Starovoitov wrote: > On Sat, Feb 19, 2022 at 6:59 PM Kumar Kartikeya Dwivedi > <memxor@xxxxxxxxx> wrote: > > > > On Sun, Feb 20, 2022 at 07:48:08AM IST, Alexei Starovoitov wrote: > > > On Sat, Feb 19, 2022 at 05:40:35PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > On Sat, Feb 19, 2022 at 05:07:39PM IST, Kumar Kartikeya Dwivedi wrote: > > > > > A few more fixes for bad PTR_TO_BTF_ID reg->off being accepted in places, that > > > > > can lead to the kernel crashing. Noticed while making sure my own series for BTF > > > > > ID pointer in map won't allow stores for pointers with incorrect offsets. > > > > > > > > > > I include one example where d_path can crash even if you NULL check > > > > > PTR_TO_BTF_ID, and one example of how missing NULL check in helper taking > > > > > PTR_TO_BTF_ID (like bpf_sock_from_file) is a real problem, see the selftest > > > > > patch. > > > > > > > > > > The &f->f_path becomes NULL + offset in case f is NULL, circumventing NULL > > > > > checks in existing helpers. The only thing needed to trigger this finding an > > > > > object that embeds the object of interest, and then somehow obtaining a NULL > > > > > PTR_TO_BTF_ID to it (not hard, esp. due to exception handler for PROBE_MEM loads > > > > > writing 0 to destination register). > > > > > > > > > > However, for the case of patch 2, it is allowed in my series since the next load > > > > > of the bad pointer stored using: > > > > > struct file *f = ...; // some pointer walking returning NULL pointer > > > > > map_val->ptr = &f->f_path; // ptr being struct path * > > > > > ... would be marked as PTR_UNTRUSTED, so it won't be allowed to be passed into > > > > > the kernel, and hence can be permitted. In referenced case, the PTR_TO_BTF_ID > > > > > should not be NULL anyway. kptr_get style helper takes PTR_TO_MAP_VALUE in > > > > > referenced ptr case only, so the load either yields NULL or RCU protected > > > > > pointer. > > > > > > > > > > Tests for patch 1 depend on fixup_kfunc_btf_id in test_verifier, hence will be > > > > > sent after merge window opens, some other changes after bpf tree merges into > > > > > bpf-next, but all pending ones can be seen here [0]. Tests for patch 2 are > > > > > included, and try to trigger crash without the fix, but it's not 100% reliable. > > > > > We may need special testing helpers or kfuncs to make it thorough, but wanted to > > > > > wait before getting feedback. > > > > > > > > > > Issue fixed by patch 2 is a bit more broader in scope, and would require proper > > > > > discussion (before being applied) on the correct way forward, as it is > > > > > technically backwards incompatible change, but hopefully never breaks real > > > > > programs, only malicious or already incorrect ones. > > > > > > > > > > Also, please suggest the right "Fixes" tag for patch 2. > > > > > > > > > > As for patch 3 (selftest), please suggest a better way to get a certain type of > > > > > PTR_TO_BTF_ID which can be NULL or NULL+offset. Can we add kfuncs for testing > > > > > that return such pointers and make them available to e.g. TC progs, if the fix > > > > > in patch 2 is acceptable? > > > > > > > > > > [0]: https://github.com/kkdwivedi/linux/commits/fixes-bpf-next > > > > > > > > > > > > > Looking at BPF CI [1], it seems it surfaces another problem I was seeing locally > > > > but couldn't craft a reliable test case for, that it forms a non-NULL but > > > > invalid pointer using pointer walking, in some cases RCU read lock provides > > > > protection for those cases, but not all (esp. if kernel doesn't clear the old > > > > pointer that was in use before, and has it sitting in some location). RDI (arg1) > > > > seems to be pointing somewhere behind the faulting address, which means the > > > > &f->f_path is bad. > > > > > > > > But this requires a larger discussion. > > > > > > > > In that case, PAGE_SIZE thing won't help. We may have to introduce a PTR_BPF_REF > > > > flag (e.g. set for ctx args of tracing progs, set based on BTF tagging in other > > > > places) which tells the verifier that the pointer for the duration of program > > > > will be valid, so it can be passed into helpers. > > > > > > > > So for cases like &f->f_path it will work, since file + off will still have > > > > PTR_BPF_REF set in reg state. In case of pointer walking, where dst_reg state > > > > is updated on walk, we may have to explicitly tag members where PTR_BPF_REF can > > > > be inherited if parent object has PTR_BPF_REF (i.e. ref to parent implies ref to > > > > child cases). > > > > > > > > Something like: > > > > > > > > struct foo { > > > > ... > > > > struct bar __bpf_ref *p; > > > > struct baz *q; > > > > ... > > > > } > > > > > > > > ... then if getting: > > > > > > > > struct foo *f = ...; // PTR_TO_BTF_ID | PTR_BPF_REF > > > > struct bar *p = f->p; // Inherits PTR_BPF_REF > > > > struct baz *q = f->q; // Does not inherit PTR_BPF_REF > > > > > > > > Thoughts? > > > > > > > > [1]: https://github.com/kernel-patches/bpf/runs/5258413028?check_suite_focus=true > > > > > > fd_array wasn't zero initialized at alloc time, so it contains garbage. > > > fd_array[63] read that garbage. > > > So patches 2 and 3 don't help. > > > The 'fixes' list for patch 3 is ridiculous. No need. > > > Pls drop patch 2 and in instead of > > > +#define bpf_ptr_is_invalid(p) (unlikely((unsigned long)(p) < PAGE_SIZE)) > > > do static inline function that checks > > > if ((unsigned long)p < user_addr_max()) > > > > This prevents this specific case, but what happens when PTR_TO_BTF_ID can be > > formed to an already freed object (which seems even more likely to me in > > sleepable progs, because typical RCU grace period won't wait for us)? Or even > > just reading from structures which don't clear pointers they have freed, but are > > themselves not freed (so exception handling zeroing dst reg won't kick in). > > Right. The above check would prevent a class of issues, but not all of them. > If kernel code doesn't clear the pointer inside a struct after freeing it > it would lead to uaf. > Thankfully most of the code is rcu and typical rcu protected pointer > usage would replace the pointer before doing call_rcu() to clean it. > There are corner cases and we need to gradually close such holes. > My point is that it's not a bpf tree material. No need to > create panic and do a patch with gadzillion 'fixes' tags. Ok, I'll drop them, but some stable helpers did miss checking NULL too, e.g. bpf_sock_from_file. > With bpf progs calling kfuncs there is a possibility of a crash. This is not just limited to kfuncs. Apart from BPF helpers, just as an example, I was looking at bpf_inode_storage last week, that looks suspect to me now; after it has been enabled in sleepable progs, __destroy_inode etc. no longer wait for the rcu_read_unlock_trace. So many more cases where inode pointer is obtained by following pointer chains (e.g. we can just consider fd_array case, where valid file is closed after we obtain file->f_inode) which would be prevented due to RCU before, would now be problematic. > We need to eliminate such possibilities when we can. > But we're not going to backport hundred patches to old kernels. > We will close one hole today and a year later another hole could be found. > We'll close it too, but we are not going to backport a year worth of > verifier patches. Ok, understood. I will target bpf-next going forward, but since you agree this needs to be fixed, it seems to me we need to bring in a hammer and mark all PTR_TO_BTF_ID not obtained from known 'safe' contexts as e.g. PTR_UNTRUSTED, so that they can only be dereferenced. But we can clear the untrusted flag for cases where we know it is guaranteed to be a valid pointer, e.g. say LSM args, atleast for the duration of the program lifetime. Martin also floated a similar idea when sleepable local storage was being discussed [0] (the paragraph describing PTR_TO_RDONLY_BTF_ID). We also need to handle cases where pointer walking is used, to decide whether to set PTR_UNTRUSTED or not. I guess we can use BTF tagging here to annotate members and say 'if valid pointer to parent exists, valid pointer to this object can be formed too', but not all cases are as simple. WDYT? [0]: https://lore.kernel.org/bpf/20210901063217.5zpvnltvfmctrkum@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > > > > and bails out. > > > bpf-next is fine. > > > Not sure whether patch 1 is strictly necessary after above change. > > > > It is still needed to prevent the var_off case. > > Right. var_off should be prevented. -- Kartikeya
