On Thu, 2024-12-12 at 01:20 -0800, Kumar Kartikeya Dwivedi wrote:
> Robert Morris reported the following program type which passes the
> verifier in [0]:
>
> SEC("struct_ops/bpf_cubic_init")
> void BPF_PROG(bpf_cubic_init, struct sock *sk)
> {
> asm volatile("r2 = *(u16*)(r1 + 0)"); // verifier should demand u64
> asm volatile("*(u32 *)(r2 +1504) = 0"); // 1280 in some configs
> }
>
> The second line may or may not work, but the first instruction shouldn't
> pass, as it's a narrow load into the context structure of the struct ops
> callback. The code falls back to btf_ctx_access to ensure correctness
> and obtaining the types of pointers. Ensure that the size of the access
> is correctly checked to be 8 bytes, otherwise the verifier thinks the
> narrow load obtained a trusted BTF pointer and will permit loads/stores
> as it sees fit.
>
> Perform the check on size after we've verified that the load is for a
> pointer field, as for scalar values narrow loads are fine. Access to
> structs passed as arguments to a BPF program are also treated as
> scalars, therefore no adjustment is needed in their case.
>
> Existing verifier selftests are broken by this change, but because they
> were incorrect. Verifier tests for d_path were performing narrow load
> into context to obtain path pointer, had this program actually run it
> would cause a crash. The same holds for verifier_btf_ctx_access tests.
>
> [0]: https://lore.kernel.org/bpf/51338.1732985814@localhost
>
> Fixes: 9e15db66136a ("bpf: Implement accurate raw_tp context access via BTF")
> Reported-by: Robert Morris <rtm@xxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> kernel/bpf/btf.c | 6 ++++++
> tools/testing/selftests/bpf/progs/verifier_btf_ctx_access.c | 4 ++--
> tools/testing/selftests/bpf/progs/verifier_d_path.c | 4 ++--
> 3 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index e7a59e6462a9..a63a03582f02 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6543,6 +6543,12 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
> return false;
> }
>
> + if (size != sizeof(u64)) {
> + bpf_log(log, "func '%s' size %d must be 8\n",
Nit: the error message is somewhat confusing.
Maybe print something like:
"func '%s' param #%d access size should be 8, not %d"?
> + tname, size);
> + return false;
> + }
> +
> /* check for PTR_TO_RDONLY_BUF_OR_NULL or PTR_TO_RDWR_BUF_OR_NULL */
> for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
> const struct bpf_ctx_arg_aux *ctx_arg_info = &prog->aux->ctx_arg_info[i];
[...]
