Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH bpf v1 0/5] More fixes for crashes due to bad PTR_TO_BTF_ID reg->off
Date: Sat, 19 Feb 2022 17:40:35 +0530	[thread overview]
Message-ID: <20220219121035.c6c5dmvbchzaqqak@apollo.legion> (raw)
In-Reply-To: <20220219113744.1852259-1-memxor@gmail.com>

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

> Kumar Kartikeya Dwivedi (5):
>   bpf: Fix kfunc register offset check for PTR_TO_BTF_ID
>   bpf: Restrict PTR_TO_BTF_ID offset to PAGE_SIZE when calling helpers
>   bpf: Use bpf_ptr_is_invalid for all helpers taking PTR_TO_BTF_ID
>   selftests/bpf: Add selftest for PTR_TO_BTF_ID NULL + off case
>   selftests/bpf: Adjust verifier selftest for updated message
>
>  include/linux/bpf.h                           | 19 ++++
>  include/linux/bpf_verifier.h                  |  3 +
>  kernel/bpf/bpf_inode_storage.c                |  4 +-
>  kernel/bpf/bpf_lsm.c                          |  4 +-
>  kernel/bpf/bpf_task_storage.c                 |  4 +-
>  kernel/bpf/btf.c                              | 24 ++++-
>  kernel/bpf/stackmap.c                         |  3 +
>  kernel/bpf/task_iter.c                        |  2 +-
>  kernel/bpf/verifier.c                         | 99 +++++++++++++------
>  kernel/trace/bpf_trace.c                      | 12 +++
>  net/core/bpf_sk_storage.c                     |  9 +-
>  net/core/filter.c                             | 52 ++++++----
>  net/ipv4/bpf_tcp_ca.c                         |  4 +-
>  .../selftests/bpf/prog_tests/d_path_crash.c   | 19 ++++
>  .../selftests/bpf/progs/d_path_crash.c        | 26 +++++
>  .../selftests/bpf/verifier/bounds_deduction.c |  2 +-
>  tools/testing/selftests/bpf/verifier/ctx.c    |  8 +-
>  17 files changed, 226 insertions(+), 68 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/d_path_crash.c
>  create mode 100644 tools/testing/selftests/bpf/progs/d_path_crash.c
>
> --
> 2.35.1
>

--
Kartikeya