Re: [PATCH bpf v1 1/2] bpf: Check size for BTF-based ctx access of pointer members

[Eduard Zingerman <eddyz87@gmail.com>]

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@mit.edu>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

>  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];

[...]