| Message ID | 20230801203630.3581291-3-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | BPF Refcount followups 3: bpf_mem_free_rcu refcounted nodes | expand |
On 8/1/23 1:36 PM, Dave Marchevsky wrote: > Recent discussions around default kptr "trustedness" led to changes such > as commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the > verifier."). One of the conclusions of those discussions, as expressed > in code and comments in that patch, is that we'd like to move away from > 'raw' PTR_TO_BTF_ID without some type flag or other register state > indicating trustedness. Although PTR_TRUSTED and PTR_UNTRUSTED flags mark > this state explicitly, the verifier currently considers trustedness > implied by other register state. For example, owning refs to graph > collection nodes must have a nonzero ref_obj_id, so they pass the > is_trusted_reg check despite having no explicit PTR_{UN}TRUSTED flag. > This patch makes trustedness of non-owning refs to graph collection > nodes explicit as well. > > By definition, non-owning refs are currently trusted. Although the ref > has no control over pointee lifetime, due to non-owning ref clobbering > rules (see invalidate_non_owning_refs) dereferencing a non-owning ref is > safe in the critical section controlled by bpf_spin_lock associated with > its owning collection. > > Note that the previous statement does not hold true for nodes with shared > ownership due to the use-after-free issue that this series is > addressing. True shared ownership was disabled by commit 7deca5eae833 > ("bpf: Disable bpf_refcount_acquire kfunc calls until race conditions are fixed"), > though, so the statement holds for now. Further patches in the series will change > the trustedness state of non-owning refs before re-enabling > bpf_refcount_acquire. > > Let's add NON_OWN_REF type flag to BPF_REG_TRUSTED_MODIFIERS such that a > non-owning ref reg state would pass is_trusted_reg check. Somewhat > surprisingly, this doesn't result in any change to user-visible > functionality elsewhere in the verifier: graph collection nodes are all > marked MEM_ALLOC, which tends to be handled in separate codepaths from > "raw" PTR_TO_BTF_ID. Regardless, let's be explicit here and document the > current state of things before changing it elsewhere in the series. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> Acked-by: Yonghong Song <yonghong.song@linux.dev>
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index f70f9ac884d2..b6e58dab8e27 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -745,7 +745,7 @@ static inline bool bpf_prog_check_recur(const struct bpf_prog *prog) } } -#define BPF_REG_TRUSTED_MODIFIERS (MEM_ALLOC | PTR_TRUSTED) +#define BPF_REG_TRUSTED_MODIFIERS (MEM_ALLOC | PTR_TRUSTED | NON_OWN_REF) static inline bool bpf_type_has_unsafe_modifiers(u32 type) {
Recent discussions around default kptr "trustedness" led to changes such as commit 6fcd486b3a0a ("bpf: Refactor RCU enforcement in the verifier."). One of the conclusions of those discussions, as expressed in code and comments in that patch, is that we'd like to move away from 'raw' PTR_TO_BTF_ID without some type flag or other register state indicating trustedness. Although PTR_TRUSTED and PTR_UNTRUSTED flags mark this state explicitly, the verifier currently considers trustedness implied by other register state. For example, owning refs to graph collection nodes must have a nonzero ref_obj_id, so they pass the is_trusted_reg check despite having no explicit PTR_{UN}TRUSTED flag. This patch makes trustedness of non-owning refs to graph collection nodes explicit as well. By definition, non-owning refs are currently trusted. Although the ref has no control over pointee lifetime, due to non-owning ref clobbering rules (see invalidate_non_owning_refs) dereferencing a non-owning ref is safe in the critical section controlled by bpf_spin_lock associated with its owning collection. Note that the previous statement does not hold true for nodes with shared ownership due to the use-after-free issue that this series is addressing. True shared ownership was disabled by commit 7deca5eae833 ("bpf: Disable bpf_refcount_acquire kfunc calls until race conditions are fixed"), though, so the statement holds for now. Further patches in the series will change the trustedness state of non-owning refs before re-enabling bpf_refcount_acquire. Let's add NON_OWN_REF type flag to BPF_REG_TRUSTED_MODIFIERS such that a non-owning ref reg state would pass is_trusted_reg check. Somewhat surprisingly, this doesn't result in any change to user-visible functionality elsewhere in the verifier: graph collection nodes are all marked MEM_ALLOC, which tends to be handled in separate codepaths from "raw" PTR_TO_BTF_ID. Regardless, let's be explicit here and document the current state of things before changing it elsewhere in the series. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- include/linux/bpf_verifier.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)