Re: [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup()

From: Joe Stringer <joe@wand.net.nz>
To: Martin KaFai Lau <kafai@fb.com>
Cc: netdev <netdev@vger.kernel.org>, Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	kernel-team@fb.com, Joe Stringer <joe@wand.net.nz>
Subject: Re: [PATCH bpf] bpf: Fix narrow load on a bpf_sock returned from sk_lookup()
Date: Sat, 9 Feb 2019 17:47:37 -0800	[thread overview]
Message-ID: <CAOftzPjON0f6UWv0pVU0sQx2eKiJBV61Afst1duKMTXJ_QCLWw@mail.gmail.com> (raw)
In-Reply-To: <20190209062554.142612-1-kafai@fb.com>

On Fri, 8 Feb 2019 at 22:27, Martin KaFai Lau <kafai@fb.com> wrote:
>
> By adding this test to test_verifier:
> {
>         "reference tracking: access sk->src_ip4 (narrow load)",
>         .insns = {
>         BPF_SK_LOOKUP,
>         BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
>         BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 3),
>         BPF_LDX_MEM(BPF_H, BPF_REG_2, BPF_REG_0, offsetof(struct bpf_sock, src_ip4) + 2),
>         BPF_MOV64_REG(BPF_REG_1, BPF_REG_6),
>         BPF_EMIT_CALL(BPF_FUNC_sk_release),
>         BPF_EXIT_INSN(),
>         },
>         .prog_type = BPF_PROG_TYPE_SCHED_CLS,
>         .result = ACCEPT,
> },
>
> The above test loads 2 bytes from sk->src_ip4 where
> sk is obtained by bpf_sk_lookup_tcp().
>
> It hits an internal verifier error from convert_ctx_accesses():
> [root@arch-fb-vm1 bpf]# ./test_verifier 665 665
> Failed to load prog 'Invalid argument'!
> 0: (b7) r2 = 0
> 1: (63) *(u32 *)(r10 -8) = r2
> 2: (7b) *(u64 *)(r10 -16) = r2
> 3: (7b) *(u64 *)(r10 -24) = r2
> 4: (7b) *(u64 *)(r10 -32) = r2
> 5: (7b) *(u64 *)(r10 -40) = r2
> 6: (7b) *(u64 *)(r10 -48) = r2
> 7: (bf) r2 = r10
> 8: (07) r2 += -48
> 9: (b7) r3 = 36
> 10: (b7) r4 = 0
> 11: (b7) r5 = 0
> 12: (85) call bpf_sk_lookup_tcp#84
> 13: (bf) r6 = r0
> 14: (15) if r0 == 0x0 goto pc+3
>  R0=sock(id=1,off=0,imm=0) R6=sock(id=1,off=0,imm=0) R10=fp0,call_-1 fp-8=????0000 fp-16=0000mmmm fp-24=mmmmmmmm fp-32=mmmmmmmm fp-40=mmmmmmmm fp-48=mmmmmmmm refs=1
> 15: (69) r2 = *(u16 *)(r0 +26)
> 16: (bf) r1 = r6
> 17: (85) call bpf_sk_release#86
> 18: (95) exit
>
> from 14 to 18: safe
> processed 20 insns (limit 131072), stack depth 48
> bpf verifier is misconfigured
> Summary: 0 PASSED, 0 SKIPPED, 1 FAILED
>
> The bpf_sock_is_valid_access() is expecting src_ip4 can be narrowly
> loaded (meaning load any 1 or 2 bytes of the src_ip4) by
> marking info->ctx_field_size.  However, this marked
> ctx_field_size is not used.  This patch fixes it.
>
> Due to the recent refactoring in test_verifier,
> this new test will be added to the bpf-next branch
> (together with the bpf_tcp_sock patchset)
> to avoid merge conflict.
>
> Fixes: c64b7983288e ("bpf: Add PTR_TO_SOCKET verifier type")
> Cc: Joe Stringer <joe@wand.net.nz>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---

Nice find, thanks.

Acked-by: Joe Stringer <joe@wand.net.nz>