[PATCH bpf 0/3] bpf, sockmap: Fix infinite recursion in sock_map_close

[PATCH bpf 0/3] bpf, sockmap: Fix infinite recursion in sock_map_close

[Jakub Sitnicki]

This patch set addresses the syzbot report in [1].

Please see patch #1 for the analysis and the fix.
Patches #2 & #3 add coverage to selftests.

I still need to address Eric's suggestion to break out of sock_map_close when we
detect that sock_map_close is calling itself. Will do it in a separate patch or
the next iteration, if there is any review feedback to address.

[1] https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/

To: bpf@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: kernel-team@cloudflare.com
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Andrii Nakryiko <andrii@kernel.org>

---
Jakub Sitnicki (3):
      bpf, sockmap: Check for any of tcp_bpf_prots when cloning a listener
      selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests
      selftests/bpf: Cover listener cloning with progs attached to sockmap

 net/ipv4/tcp_bpf.c                                 |  3 +-
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 81 +++++++++++++++++-----
 2 files changed, 64 insertions(+), 20 deletions(-)
---
base-commit: e7895f017b79410bf4591396a733b876dc1e0e9d
change-id: 20230113-sockmap-fix-83d0359301b7

Best regards,
-- 
Jakub Sitnicki <jakub@cloudflare.com>

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

[Jakub Sitnicki]

A listening socket linked to a sockmap has its sk_prot overridden. It
points to one of the struct proto variants in tcp_bpf_prots. The variant
depends on the socket's family and which sockmap programs are attached.

A child socket cloned from a TCP listener initially inherits their sk_prot.
But before cloning is finished, we restore the child's proto to the
listener's original non-tcp_bpf_prots one. This happens in
tcp_create_openreq_child -> tcp_bpf_clone.

Today, in tcp_bpf_clone we detect if the child's proto should be restored
by checking only for the TCP_BPF_BASE proto variant. This is not
correct. The sk_prot of listening socket linked to a sockmap can point to
to any variant in tcp_bpf_prots.

If the listeners sk_prot happens to be not the TCP_BPF_BASE variant, then
the child socket unintentionally is left if the inherited sk_prot by
tcp_bpf_clone.

This leads to issues like infinite recursion on close [1], because the
child state is otherwise not set up for use with tcp_bpf_prot operations.

Adjust the check in tcp_bpf_clone to detect all of tcp_bpf_prots variants.

Note that it wouldn't be sufficient to check the socket state when
overriding the sk_prot in tcp_bpf_update_proto in order to always use the
TCP_BPF_BASE variant for listening sockets. Since commit
b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
it is possible for a socket to transition to TCP_LISTEN state while already
linked to a sockmap, e.g. connect() -> insert into map ->
connect(AF_UNSPEC) -> listen().

[1]: https://lore.kernel.org/all/00000000000073b14905ef2e7401@google.com/

Fixes: e80251555f0b ("tcp_bpf: Don't let child socket inherit parent protocol ops on copy")
Reported-by: syzbot+04c21ed96d861dccc5cd@syzkaller.appspotmail.com
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/ipv4/tcp_bpf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
index 94aad3870c5f..8db00647e0a4 100644
--- a/net/ipv4/tcp_bpf.c
+++ b/net/ipv4/tcp_bpf.c
@@ -639,10 +639,9 @@ EXPORT_SYMBOL_GPL(tcp_bpf_update_proto);
  */
 void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
 {
-	int family = sk->sk_family == AF_INET6 ? TCP_BPF_IPV6 : TCP_BPF_IPV4;
 	struct proto *prot = newsk->sk_prot;
 
-	if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE])
+	if (tcp_bpf_prots[0] <= prot && prot < tcp_bpf_prots[ARRAY_SIZE(tcp_bpf_prots)])
 		newsk->sk_prot = sk->sk_prot_creator;
 }
 #endif /* CONFIG_BPF_SYSCALL */

-- 
2.39.0

[PATCH bpf 2/3] selftests/bpf: Pass BPF skeleton to sockmap_listen ops tests

[Jakub Sitnicki]

Following patch extends the sockmap ops tests to cover the scenario when a
sockmap with attached programs holds listening sockets.

Pass the BPF skeleton to sockmap ops test so that the can access and attach
the BPF programs.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 55 +++++++++++++++-------
 1 file changed, 37 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 2cf0c7a3fe23..499fba8f55b9 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -30,6 +30,8 @@
 #define MAX_STRERR_LEN 256
 #define MAX_TEST_NAME 80
 
+#define __always_unused	__attribute__((__unused__))
+
 #define _FAIL(errnum, fmt...)                                                  \
 	({                                                                     \
 		error_at_line(0, (errnum), __func__, __LINE__, fmt);           \
@@ -321,7 +323,8 @@ static int socket_loopback(int family, int sotype)
 	return socket_loopback_reuseport(family, sotype, -1);
 }
 
-static void test_insert_invalid(int family, int sotype, int mapfd)
+static void test_insert_invalid(struct test_sockmap_listen *skel __always_unused,
+				int family, int sotype, int mapfd)
 {
 	u32 key = 0;
 	u64 value;
@@ -338,7 +341,8 @@ static void test_insert_invalid(int family, int sotype, int mapfd)
 		FAIL_ERRNO("map_update: expected EBADF");
 }
 
-static void test_insert_opened(int family, int sotype, int mapfd)
+static void test_insert_opened(struct test_sockmap_listen *skel __always_unused,
+			       int family, int sotype, int mapfd)
 {
 	u32 key = 0;
 	u64 value;
@@ -359,7 +363,8 @@ static void test_insert_opened(int family, int sotype, int mapfd)
 	xclose(s);
 }
 
-static void test_insert_bound(int family, int sotype, int mapfd)
+static void test_insert_bound(struct test_sockmap_listen *skel __always_unused,
+			      int family, int sotype, int mapfd)
 {
 	struct sockaddr_storage addr;
 	socklen_t len;
@@ -386,7 +391,8 @@ static void test_insert_bound(int family, int sotype, int mapfd)
 	xclose(s);
 }
 
-static void test_insert(int family, int sotype, int mapfd)
+static void test_insert(struct test_sockmap_listen *skel __always_unused,
+			int family, int sotype, int mapfd)
 {
 	u64 value;
 	u32 key;
@@ -402,7 +408,8 @@ static void test_insert(int family, int sotype, int mapfd)
 	xclose(s);
 }
 
-static void test_delete_after_insert(int family, int sotype, int mapfd)
+static void test_delete_after_insert(struct test_sockmap_listen *skel __always_unused,
+				     int family, int sotype, int mapfd)
 {
 	u64 value;
 	u32 key;
@@ -419,7 +426,8 @@ static void test_delete_after_insert(int family, int sotype, int mapfd)
 	xclose(s);
 }
 
-static void test_delete_after_close(int family, int sotype, int mapfd)
+static void test_delete_after_close(struct test_sockmap_listen *skel __always_unused,
+				    int family, int sotype, int mapfd)
 {
 	int err, s;
 	u64 value;
@@ -442,7 +450,8 @@ static void test_delete_after_close(int family, int sotype, int mapfd)
 		FAIL_ERRNO("map_delete: expected EINVAL/EINVAL");
 }
 
-static void test_lookup_after_insert(int family, int sotype, int mapfd)
+static void test_lookup_after_insert(struct test_sockmap_listen *skel __always_unused,
+				     int family, int sotype, int mapfd)
 {
 	u64 cookie, value;
 	socklen_t len;
@@ -470,7 +479,8 @@ static void test_lookup_after_insert(int family, int sotype, int mapfd)
 	xclose(s);
 }
 
-static void test_lookup_after_delete(int family, int sotype, int mapfd)
+static void test_lookup_after_delete(struct test_sockmap_listen *skel __always_unused,
+				     int family, int sotype, int mapfd)
 {
 	int err, s;
 	u64 value;
@@ -493,7 +503,8 @@ static void test_lookup_after_delete(int family, int sotype, int mapfd)
 	xclose(s);
 }
 
-static void test_lookup_32_bit_value(int family, int sotype, int mapfd)
+static void test_lookup_32_bit_value(struct test_sockmap_listen *skel __always_unused,
+				     int family, int sotype, int mapfd)
 {
 	u32 key, value32;
 	int err, s;
@@ -523,7 +534,8 @@ static void test_lookup_32_bit_value(int family, int sotype, int mapfd)
 	xclose(s);
 }
 
-static void test_update_existing(int family, int sotype, int mapfd)
+static void test_update_existing(struct test_sockmap_listen *skel __always_unused,
+				 int family, int sotype, int mapfd)
 {
 	int s1, s2;
 	u64 value;
@@ -551,7 +563,8 @@ static void test_update_existing(int family, int sotype, int mapfd)
 /* Exercise the code path where we destroy child sockets that never
  * got accept()'ed, aka orphans, when parent socket gets closed.
  */
-static void test_destroy_orphan_child(int family, int sotype, int mapfd)
+static void test_destroy_orphan_child(struct test_sockmap_listen *skel __always_unused,
+				      int family, int sotype, int mapfd)
 {
 	struct sockaddr_storage addr;
 	socklen_t len;
@@ -585,7 +598,8 @@ static void test_destroy_orphan_child(int family, int sotype, int mapfd)
 /* Perform a passive open after removing listening socket from SOCKMAP
  * to ensure that callbacks get restored properly.
  */
-static void test_clone_after_delete(int family, int sotype, int mapfd)
+static void test_clone_after_delete(struct test_sockmap_listen *skel __always_unused,
+				    int family, int sotype, int mapfd)
 {
 	struct sockaddr_storage addr;
 	socklen_t len;
@@ -621,7 +635,8 @@ static void test_clone_after_delete(int family, int sotype, int mapfd)
  * SOCKMAP, but got accept()'ed only after the parent has been removed
  * from SOCKMAP, gets cloned without parent psock state or callbacks.
  */
-static void test_accept_after_delete(int family, int sotype, int mapfd)
+static void test_accept_after_delete(struct test_sockmap_listen *skel __always_unused,
+				     int family, int sotype, int mapfd)
 {
 	struct sockaddr_storage addr;
 	const u32 zero = 0;
@@ -675,7 +690,8 @@ static void test_accept_after_delete(int family, int sotype, int mapfd)
 /* Check that child socket that got created and accepted while parent
  * was in a SOCKMAP is cloned without parent psock state or callbacks.
  */
-static void test_accept_before_delete(int family, int sotype, int mapfd)
+static void test_accept_before_delete(struct test_sockmap_listen *skel __always_unused,
+				      int family, int sotype, int mapfd)
 {
 	struct sockaddr_storage addr;
 	const u32 zero = 0, one = 1;
@@ -784,7 +800,8 @@ static void *connect_accept_thread(void *arg)
 	return NULL;
 }
 
-static void test_syn_recv_insert_delete(int family, int sotype, int mapfd)
+static void test_syn_recv_insert_delete(struct test_sockmap_listen *skel __always_unused,
+					int family, int sotype, int mapfd)
 {
 	struct connect_accept_ctx ctx = { 0 };
 	struct sockaddr_storage addr;
@@ -847,7 +864,8 @@ static void *listen_thread(void *arg)
 	return NULL;
 }
 
-static void test_race_insert_listen(int family, int socktype, int mapfd)
+static void test_race_insert_listen(struct test_sockmap_listen *skel __always_unused,
+				    int family, int socktype, int mapfd)
 {
 	struct connect_accept_ctx ctx = { 0 };
 	const u32 zero = 0;
@@ -1473,7 +1491,8 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map,
 		     int family, int sotype)
 {
 	const struct op_test {
-		void (*fn)(int family, int sotype, int mapfd);
+		void (*fn)(struct test_sockmap_listen *skel,
+			   int family, int sotype, int mapfd);
 		const char *name;
 		int sotype;
 	} tests[] = {
@@ -1520,7 +1539,7 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map,
 		if (!test__start_subtest(s))
 			continue;
 
-		t->fn(family, sotype, map_fd);
+		t->fn(skel, family, sotype, map_fd);
 		test_ops_cleanup(map);
 	}
 }

-- 
2.39.0

[PATCH bpf 3/3] selftests/bpf: Cover listener cloning with progs attached to sockmap

[Jakub Sitnicki]

Today we test if a child socket is cloned properly from a listening socket
inside a sockmap only when there are no BPF programs attached to the map.

A bug has been reported [1] for the case when sockmap has a verdict program
attached. So cover this case as well to prevent regressions.

[1]: https://lore.kernel.org/r/00000000000073b14905ef2e7401@google.com

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 .../selftests/bpf/prog_tests/sockmap_listen.c      | 30 ++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
index 499fba8f55b9..567e07c19ecc 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockmap_listen.c
@@ -563,8 +563,7 @@ static void test_update_existing(struct test_sockmap_listen *skel __always_unuse
 /* Exercise the code path where we destroy child sockets that never
  * got accept()'ed, aka orphans, when parent socket gets closed.
  */
-static void test_destroy_orphan_child(struct test_sockmap_listen *skel __always_unused,
-				      int family, int sotype, int mapfd)
+static void do_destroy_orphan_child(int family, int sotype, int mapfd)
 {
 	struct sockaddr_storage addr;
 	socklen_t len;
@@ -595,6 +594,33 @@ static void test_destroy_orphan_child(struct test_sockmap_listen *skel __always_
 	xclose(s);
 }
 
+static void test_destroy_orphan_child(struct test_sockmap_listen *skel,
+				      int family, int sotype, int mapfd)
+{
+	int msg_verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
+	int skb_verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
+	const struct test {
+		int progfd;
+		enum bpf_attach_type atype;
+	} tests[] = {
+		{ -1, -1 },
+		{ msg_verdict, BPF_SK_MSG_VERDICT },
+		{ skb_verdict, BPF_SK_SKB_VERDICT },
+	};
+	const struct test *t;
+
+	for (t = tests; t < tests + ARRAY_SIZE(tests); t++) {
+		if (t->progfd != -1 &&
+		    xbpf_prog_attach(t->progfd, mapfd, t->atype, 0) != 0)
+			return;
+
+		do_destroy_orphan_child(family, sotype, mapfd);
+
+		if (t->progfd != -1)
+			xbpf_prog_detach2(t->progfd, mapfd, t->atype);
+	}
+}
+
 /* Perform a passive open after removing listening socket from SOCKMAP
  * to ensure that callbacks get restored properly.
  */

-- 
2.39.0

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

[Dan Carpenter]

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.


-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Jakub Sitnicki]

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 :-)

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

[Eric Dumazet]

On Mon, Jan 16, 2023 at 11:13 AM Jakub Sitnicki <jakub@cloudflare.com> wrote:
>
> 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 :-)

We might add a generic helper to make all this a bit more clear ?

+#define is_insidevar(PTR, VAR) (                       \
+       ((void *)(PTR) <= (void *)(VAR)) &&             \
+       ((void *)(PTR) <= (void *)(VAR) + sizeof(VAR)))
+


...

if (is_insidevar(prot, tcp_bpf_prots))
     newsk->sk_prot = sk->sk_prot_creator;

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

[Dan Carpenter]

On Mon, Jan 16, 2023 at 11:09:02AM +0100, Jakub Sitnicki wrote:
> 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;

Huh.  I can silence this false positive in Smatch...  It never even
occured to me that this was a two dimensional array (I only have the
information in the email).

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

Heh.  I must have an older version of Clang.

  CC      net/ipv4/tcp_bpf.o
net/ipv4/tcp_bpf.c:644:41: warning: array index 2 is past the end of the array (that has type 'struct proto[2][4]') [-Warray-bounds]
        if (tcp_bpf_prots[0] <= prot && prot < tcp_bpf_prots[ARRAY_SIZE(tcp_bpf_prots)])
                                               ^             ~~~~~~~~~~~~~~~~~~~~~~~~~
net/ipv4/tcp_bpf.c:544:1: note: array 'tcp_bpf_prots' declared here
static struct proto tcp_bpf_prots[TCP_BPF_NUM_PROTS][TCP_BPF_NUM_CFGS];
^
1 warning generated.

regards,
dan carpetner

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

[Jakub Sitnicki]

On Mon, Jan 16, 2023 at 11:39 AM +01, Eric Dumazet wrote:
> We might add a generic helper to make all this a bit more clear ?
>
> +#define is_insidevar(PTR, VAR) (                       \
> +       ((void *)(PTR) <= (void *)(VAR)) &&             \
> +       ((void *)(PTR) <= (void *)(VAR) + sizeof(VAR)))
> +
>
>
> ...
>
> if (is_insidevar(prot, tcp_bpf_prots))
>      newsk->sk_prot = sk->sk_prot_creator;

Sure can do. Thanks for the suggestion. I adjusted it a bit:
 - added cast to char * so we don't offend -Wpointer-arith,
 - fixed the lower and upper bound check.

Final form would look like:

#define is_insidevar(ptr, var)						\
	((void *)(ptr) >= (void *)(var)) &&				\
	((void *)(ptr) <  (void *)((char *)(var) + sizeof(var)))

Not sure where to stuff it. I propose include/linux/util_macros.h.

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

[Jakub Sitnicki]

On Mon, Jan 16, 2023 at 02:13 PM +03, Dan Carpenter wrote:
> On Mon, Jan 16, 2023 at 11:09:02AM +0100, Jakub Sitnicki wrote:

[...]

>> 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;
>
> Huh.  I can silence this false positive in Smatch...  It never even
> occured to me that this was a two dimensional array (I only have the
> information in the email).
>

No need. Eric's macro helper makes Smatch happy. I'll use it in v2.

>> 
>> Clang can do pointer arithmetic on 2D arrays just fine :-)
>
> Heh.  I must have an older version of Clang.
>
>   CC      net/ipv4/tcp_bpf.o
> net/ipv4/tcp_bpf.c:644:41: warning: array index 2 is past the end of the array (that has type 'struct proto[2][4]') [-Warray-bounds]
>         if (tcp_bpf_prots[0] <= prot && prot < tcp_bpf_prots[ARRAY_SIZE(tcp_bpf_prots)])
>                                                ^             ~~~~~~~~~~~~~~~~~~~~~~~~~
> net/ipv4/tcp_bpf.c:544:1: note: array 'tcp_bpf_prots' declared here
> static struct proto tcp_bpf_prots[TCP_BPF_NUM_PROTS][TCP_BPF_NUM_CFGS];
> ^
> 1 warning generated.

FWIW, I've checked against:

$ clang --version
clang version 15.0.6 (Fedora 15.0.6-2.fc37)

Gotta keep it fresh to be able to build bpf selftests ;-)
But I sure don't want to break builds with older Clangs.

Thanks for pointing it out.

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

[Dan Carpenter]

On Mon, Jan 16, 2023 at 12:31:11PM +0100, Jakub Sitnicki wrote:
> >> Clang can do pointer arithmetic on 2D arrays just fine :-)
> >
> > Heh.  I must have an older version of Clang.
> >
> >   CC      net/ipv4/tcp_bpf.o
> > net/ipv4/tcp_bpf.c:644:41: warning: array index 2 is past the end of the array (that has type 'struct proto[2][4]') [-Warray-bounds]
> >         if (tcp_bpf_prots[0] <= prot && prot < tcp_bpf_prots[ARRAY_SIZE(tcp_bpf_prots)])
> >                                                ^             ~~~~~~~~~~~~~~~~~~~~~~~~~
> > net/ipv4/tcp_bpf.c:544:1: note: array 'tcp_bpf_prots' declared here
> > static struct proto tcp_bpf_prots[TCP_BPF_NUM_PROTS][TCP_BPF_NUM_CFGS];
> > ^
> > 1 warning generated.
> 
> FWIW, I've checked against:
> 
> $ clang --version
> clang version 15.0.6 (Fedora 15.0.6-2.fc37)
> 
> Gotta keep it fresh to be able to build bpf selftests ;-)
> But I sure don't want to break builds with older Clangs.

I'm actually on a newer 16.x something version from git.

Btw, it made me outrageously happy that Clang was one for one bug
compatible with Smatch on this.

With this kind of warning you could either print a warning when there is
a read but that's not what either Smatch or Clang do.  Smatch looks at
the offset and then checks to see if the code is just doing pointer
math to find the &(array + 1) address.

So Smatch checks is the offset known to be exactly ARRAY_SIZE() and are
we taking the address of that.  I have updated that check.

regards,
dan carpenter