Re: [PATCH bpf 1/3] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener

[Jakub Sitnicki <jakub@cloudflare.com>]

Hi Dan,

On Sat, Jan 14, 2023 at 11:04 AM +03, Dan Carpenter wrote:
> Hi Jakub,
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Jakub-Sitnicki/bpf-sockmap-Check-for-any-of-tcp_bpf_prots-when-cloning-a-listener/20230113-230728
> base:   e7895f017b79410bf4591396a733b876dc1e0e9d
> patch link:    https://lore.kernel.org/r/20230113-sockmap-fix-v1-1-d3cad092ee10%40cloudflare.com
> patch subject: [PATCH bpf 1/3] bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener
> config: i386-randconfig-m021
> compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <error27@gmail.com>
>
> smatch warnings:
> net/ipv4/tcp_bpf.c:644 tcp_bpf_clone() error: buffer overflow 'tcp_bpf_prots' 2 <= 2
>
> vim +/tcp_bpf_prots +644 net/ipv4/tcp_bpf.c
>
> 604326b41a6fb9 Daniel Borkmann 2018-10-13  634  
> e80251555f0bef Jakub Sitnicki  2020-02-18  635  /* If a child got cloned from a listening socket that had tcp_bpf
> e80251555f0bef Jakub Sitnicki  2020-02-18  636   * protocol callbacks installed, we need to restore the callbacks to
> e80251555f0bef Jakub Sitnicki  2020-02-18  637   * the default ones because the child does not inherit the psock state
> e80251555f0bef Jakub Sitnicki  2020-02-18  638   * that tcp_bpf callbacks expect.
> e80251555f0bef Jakub Sitnicki  2020-02-18  639   */
> e80251555f0bef Jakub Sitnicki  2020-02-18  640  void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
> e80251555f0bef Jakub Sitnicki  2020-02-18  641  {
> e80251555f0bef Jakub Sitnicki  2020-02-18  642  	struct proto *prot = newsk->sk_prot;
> e80251555f0bef Jakub Sitnicki  2020-02-18  643  
> c2e74657613125 Jakub Sitnicki  2023-01-13 @644  	if (tcp_bpf_prots[0] <= prot && prot < tcp_bpf_prots[ARRAY_SIZE(tcp_bpf_prots)])
>                                                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> What?  Also I suspect this might cause a compile error for Clang builds.

Can't say I see a problem B-)

tcp_bpf_prots is a 2D array:

static struct proto tcp_bpf_prots[TCP_BPF_NUM_PROTS][TCP_BPF_NUM_CFGS];

... so tcp_bpf_prots[0] is the base address of the first array, while
tcp_bpf_prots[ARRAY_SIZE(tcp_bpf_prots)] is the base address of the
array one past the last one.

Smatch doesn't seem to graps the 2D array concept here. We can make it
happy by being explicit but harder on the eyes:

	if (&tcp_bpf_prots[0][0] <= prot && prot < &tcp_bpf_prots[ARRAY_SIZE(tcp_bpf_prots)][0])
		newsk->sk_prot = sk->sk_prot_creator;

Clang can do pointer arithmetic on 2D arrays just fine :-)