Skip to content

Commit e8c8fd9

Browse files
author
Alexei Starovoitov
committed
Merge branch 'bpf, sockmap: Fix infinite recursion in sock_map_close'
Jakub Sitnicki says: ==================== This patch set addresses the syzbot report in [1]. Patch #1 has been suggested by Eric [2]. I extended it to cover the rest of sock_map proto callbacks. Otherwise we would still overflow the stack. Patch #2 contains the actual fix and bug analysis. Patches #3 & #4 add coverage to selftests to trigger the bug. [1] https://lore.kernel.org/all/[email protected]/ [2] https://lore.kernel.org/all/CANn89iK2UN1FmdUcH12fv_xiZkv2G+Nskvmq7fG6aA_6VKRf6g@mail.gmail.com/ --- v1 -> v2: v1: https://lore.kernel.org/r/[email protected] [v1 didn't hit bpf@ ML by mistake] * pull in Eric's patch to protect against recursion loop bugs (Eric) * add a macro helper to check if pointer is inside a memory range (Eric) ==================== Signed-off-by: Alexei Starovoitov <[email protected]>
2 parents 74bc3a5 + c88ea16 commit e8c8fd9

File tree

4 files changed

+111
-47
lines changed

4 files changed

+111
-47
lines changed

Diff for: ‎include/linux/util_macros.h

+12
Original file line numberDiff line numberDiff line change
@@ -38,4 +38,16 @@
3838
*/
3939
#define find_closest_descending(x, a, as) __find_closest(x, a, as, >=)
4040

41+
/**
42+
* is_insidevar - check if the @ptr points inside the @var memory range.
43+
* @ptr: the pointer to a memory address.
44+
* @var: the variable which address and size identify the memory range.
45+
*
46+
* Evaluates to true if the address in @ptr lies within the memory
47+
* range allocated to @var.
48+
*/
49+
#define is_insidevar(ptr, var) \
50+
((uintptr_t)(ptr) >= (uintptr_t)(var) && \
51+
(uintptr_t)(ptr) < (uintptr_t)(var) + sizeof(var))
52+
4153
#endif

Diff for: ‎net/core/sock_map.c

+34-27
Original file line numberDiff line numberDiff line change
@@ -1569,15 +1569,16 @@ void sock_map_unhash(struct sock *sk)
15691569
psock = sk_psock(sk);
15701570
if (unlikely(!psock)) {
15711571
rcu_read_unlock();
1572-
if (sk->sk_prot->unhash)
1573-
sk->sk_prot->unhash(sk);
1574-
return;
1572+
saved_unhash = READ_ONCE(sk->sk_prot)->unhash;
1573+
} else {
1574+
saved_unhash = psock->saved_unhash;
1575+
sock_map_remove_links(sk, psock);
1576+
rcu_read_unlock();
15751577
}
1576-
1577-
saved_unhash = psock->saved_unhash;
1578-
sock_map_remove_links(sk, psock);
1579-
rcu_read_unlock();
1580-
saved_unhash(sk);
1578+
if (WARN_ON_ONCE(saved_unhash == sock_map_unhash))
1579+
return;
1580+
if (saved_unhash)
1581+
saved_unhash(sk);
15811582
}
15821583
EXPORT_SYMBOL_GPL(sock_map_unhash);
15831584

@@ -1590,17 +1591,18 @@ void sock_map_destroy(struct sock *sk)
15901591
psock = sk_psock_get(sk);
15911592
if (unlikely(!psock)) {
15921593
rcu_read_unlock();
1593-
if (sk->sk_prot->destroy)
1594-
sk->sk_prot->destroy(sk);
1595-
return;
1594+
saved_destroy = READ_ONCE(sk->sk_prot)->destroy;
1595+
} else {
1596+
saved_destroy = psock->saved_destroy;
1597+
sock_map_remove_links(sk, psock);
1598+
rcu_read_unlock();
1599+
sk_psock_stop(psock);
1600+
sk_psock_put(sk, psock);
15961601
}
1597-
1598-
saved_destroy = psock->saved_destroy;
1599-
sock_map_remove_links(sk, psock);
1600-
rcu_read_unlock();
1601-
sk_psock_stop(psock);
1602-
sk_psock_put(sk, psock);
1603-
saved_destroy(sk);
1602+
if (WARN_ON_ONCE(saved_destroy == sock_map_destroy))
1603+
return;
1604+
if (saved_destroy)
1605+
saved_destroy(sk);
16041606
}
16051607
EXPORT_SYMBOL_GPL(sock_map_destroy);
16061608

@@ -1615,16 +1617,21 @@ void sock_map_close(struct sock *sk, long timeout)
16151617
if (unlikely(!psock)) {
16161618
rcu_read_unlock();
16171619
release_sock(sk);
1618-
return sk->sk_prot->close(sk, timeout);
1620+
saved_close = READ_ONCE(sk->sk_prot)->close;
1621+
} else {
1622+
saved_close = psock->saved_close;
1623+
sock_map_remove_links(sk, psock);
1624+
rcu_read_unlock();
1625+
sk_psock_stop(psock);
1626+
release_sock(sk);
1627+
cancel_work_sync(&psock->work);
1628+
sk_psock_put(sk, psock);
16191629
}
1620-
1621-
saved_close = psock->saved_close;
1622-
sock_map_remove_links(sk, psock);
1623-
rcu_read_unlock();
1624-
sk_psock_stop(psock);
1625-
release_sock(sk);
1626-
cancel_work_sync(&psock->work);
1627-
sk_psock_put(sk, psock);
1630+
/* Make sure we do not recurse. This is a bug.
1631+
* Leak the socket instead of crashing on a stack overflow.
1632+
*/
1633+
if (WARN_ON_ONCE(saved_close == sock_map_close))
1634+
return;
16281635
saved_close(sk, timeout);
16291636
}
16301637
EXPORT_SYMBOL_GPL(sock_map_close);

Diff for: ‎net/ipv4/tcp_bpf.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include <linux/bpf.h>
77
#include <linux/init.h>
88
#include <linux/wait.h>
9+
#include <linux/util_macros.h>
910

1011
#include <net/inet_common.h>
1112
#include <net/tls.h>
@@ -639,10 +640,9 @@ EXPORT_SYMBOL_GPL(tcp_bpf_update_proto);
639640
*/
640641
void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
641642
{
642-
int family = sk->sk_family == AF_INET6 ? TCP_BPF_IPV6 : TCP_BPF_IPV4;
643643
struct proto *prot = newsk->sk_prot;
644644

645-
if (prot == &tcp_bpf_prots[family][TCP_BPF_BASE])
645+
if (is_insidevar(prot, tcp_bpf_prots))
646646
newsk->sk_prot = sk->sk_prot_creator;
647647
}
648648
#endif /* CONFIG_BPF_SYSCALL */

Diff for: ‎tools/testing/selftests/bpf/prog_tests/sockmap_listen.c

+63-18
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@
3030
#define MAX_STRERR_LEN 256
3131
#define MAX_TEST_NAME 80
3232

33+
#define __always_unused __attribute__((__unused__))
34+
3335
#define _FAIL(errnum, fmt...) \
3436
({ \
3537
error_at_line(0, (errnum), __func__, __LINE__, fmt); \
@@ -321,7 +323,8 @@ static int socket_loopback(int family, int sotype)
321323
return socket_loopback_reuseport(family, sotype, -1);
322324
}
323325

324-
static void test_insert_invalid(int family, int sotype, int mapfd)
326+
static void test_insert_invalid(struct test_sockmap_listen *skel __always_unused,
327+
int family, int sotype, int mapfd)
325328
{
326329
u32 key = 0;
327330
u64 value;
@@ -338,7 +341,8 @@ static void test_insert_invalid(int family, int sotype, int mapfd)
338341
FAIL_ERRNO("map_update: expected EBADF");
339342
}
340343

341-
static void test_insert_opened(int family, int sotype, int mapfd)
344+
static void test_insert_opened(struct test_sockmap_listen *skel __always_unused,
345+
int family, int sotype, int mapfd)
342346
{
343347
u32 key = 0;
344348
u64 value;
@@ -359,7 +363,8 @@ static void test_insert_opened(int family, int sotype, int mapfd)
359363
xclose(s);
360364
}
361365

362-
static void test_insert_bound(int family, int sotype, int mapfd)
366+
static void test_insert_bound(struct test_sockmap_listen *skel __always_unused,
367+
int family, int sotype, int mapfd)
363368
{
364369
struct sockaddr_storage addr;
365370
socklen_t len;
@@ -386,7 +391,8 @@ static void test_insert_bound(int family, int sotype, int mapfd)
386391
xclose(s);
387392
}
388393

389-
static void test_insert(int family, int sotype, int mapfd)
394+
static void test_insert(struct test_sockmap_listen *skel __always_unused,
395+
int family, int sotype, int mapfd)
390396
{
391397
u64 value;
392398
u32 key;
@@ -402,7 +408,8 @@ static void test_insert(int family, int sotype, int mapfd)
402408
xclose(s);
403409
}
404410

405-
static void test_delete_after_insert(int family, int sotype, int mapfd)
411+
static void test_delete_after_insert(struct test_sockmap_listen *skel __always_unused,
412+
int family, int sotype, int mapfd)
406413
{
407414
u64 value;
408415
u32 key;
@@ -419,7 +426,8 @@ static void test_delete_after_insert(int family, int sotype, int mapfd)
419426
xclose(s);
420427
}
421428

422-
static void test_delete_after_close(int family, int sotype, int mapfd)
429+
static void test_delete_after_close(struct test_sockmap_listen *skel __always_unused,
430+
int family, int sotype, int mapfd)
423431
{
424432
int err, s;
425433
u64 value;
@@ -442,7 +450,8 @@ static void test_delete_after_close(int family, int sotype, int mapfd)
442450
FAIL_ERRNO("map_delete: expected EINVAL/EINVAL");
443451
}
444452

445-
static void test_lookup_after_insert(int family, int sotype, int mapfd)
453+
static void test_lookup_after_insert(struct test_sockmap_listen *skel __always_unused,
454+
int family, int sotype, int mapfd)
446455
{
447456
u64 cookie, value;
448457
socklen_t len;
@@ -470,7 +479,8 @@ static void test_lookup_after_insert(int family, int sotype, int mapfd)
470479
xclose(s);
471480
}
472481

473-
static void test_lookup_after_delete(int family, int sotype, int mapfd)
482+
static void test_lookup_after_delete(struct test_sockmap_listen *skel __always_unused,
483+
int family, int sotype, int mapfd)
474484
{
475485
int err, s;
476486
u64 value;
@@ -493,7 +503,8 @@ static void test_lookup_after_delete(int family, int sotype, int mapfd)
493503
xclose(s);
494504
}
495505

496-
static void test_lookup_32_bit_value(int family, int sotype, int mapfd)
506+
static void test_lookup_32_bit_value(struct test_sockmap_listen *skel __always_unused,
507+
int family, int sotype, int mapfd)
497508
{
498509
u32 key, value32;
499510
int err, s;
@@ -523,7 +534,8 @@ static void test_lookup_32_bit_value(int family, int sotype, int mapfd)
523534
xclose(s);
524535
}
525536

526-
static void test_update_existing(int family, int sotype, int mapfd)
537+
static void test_update_existing(struct test_sockmap_listen *skel __always_unused,
538+
int family, int sotype, int mapfd)
527539
{
528540
int s1, s2;
529541
u64 value;
@@ -551,7 +563,7 @@ static void test_update_existing(int family, int sotype, int mapfd)
551563
/* Exercise the code path where we destroy child sockets that never
552564
* got accept()'ed, aka orphans, when parent socket gets closed.
553565
*/
554-
static void test_destroy_orphan_child(int family, int sotype, int mapfd)
566+
static void do_destroy_orphan_child(int family, int sotype, int mapfd)
555567
{
556568
struct sockaddr_storage addr;
557569
socklen_t len;
@@ -582,10 +594,38 @@ static void test_destroy_orphan_child(int family, int sotype, int mapfd)
582594
xclose(s);
583595
}
584596

597+
static void test_destroy_orphan_child(struct test_sockmap_listen *skel,
598+
int family, int sotype, int mapfd)
599+
{
600+
int msg_verdict = bpf_program__fd(skel->progs.prog_msg_verdict);
601+
int skb_verdict = bpf_program__fd(skel->progs.prog_skb_verdict);
602+
const struct test {
603+
int progfd;
604+
enum bpf_attach_type atype;
605+
} tests[] = {
606+
{ -1, -1 },
607+
{ msg_verdict, BPF_SK_MSG_VERDICT },
608+
{ skb_verdict, BPF_SK_SKB_VERDICT },
609+
};
610+
const struct test *t;
611+
612+
for (t = tests; t < tests + ARRAY_SIZE(tests); t++) {
613+
if (t->progfd != -1 &&
614+
xbpf_prog_attach(t->progfd, mapfd, t->atype, 0) != 0)
615+
return;
616+
617+
do_destroy_orphan_child(family, sotype, mapfd);
618+
619+
if (t->progfd != -1)
620+
xbpf_prog_detach2(t->progfd, mapfd, t->atype);
621+
}
622+
}
623+
585624
/* Perform a passive open after removing listening socket from SOCKMAP
586625
* to ensure that callbacks get restored properly.
587626
*/
588-
static void test_clone_after_delete(int family, int sotype, int mapfd)
627+
static void test_clone_after_delete(struct test_sockmap_listen *skel __always_unused,
628+
int family, int sotype, int mapfd)
589629
{
590630
struct sockaddr_storage addr;
591631
socklen_t len;
@@ -621,7 +661,8 @@ static void test_clone_after_delete(int family, int sotype, int mapfd)
621661
* SOCKMAP, but got accept()'ed only after the parent has been removed
622662
* from SOCKMAP, gets cloned without parent psock state or callbacks.
623663
*/
624-
static void test_accept_after_delete(int family, int sotype, int mapfd)
664+
static void test_accept_after_delete(struct test_sockmap_listen *skel __always_unused,
665+
int family, int sotype, int mapfd)
625666
{
626667
struct sockaddr_storage addr;
627668
const u32 zero = 0;
@@ -675,7 +716,8 @@ static void test_accept_after_delete(int family, int sotype, int mapfd)
675716
/* Check that child socket that got created and accepted while parent
676717
* was in a SOCKMAP is cloned without parent psock state or callbacks.
677718
*/
678-
static void test_accept_before_delete(int family, int sotype, int mapfd)
719+
static void test_accept_before_delete(struct test_sockmap_listen *skel __always_unused,
720+
int family, int sotype, int mapfd)
679721
{
680722
struct sockaddr_storage addr;
681723
const u32 zero = 0, one = 1;
@@ -784,7 +826,8 @@ static void *connect_accept_thread(void *arg)
784826
return NULL;
785827
}
786828

787-
static void test_syn_recv_insert_delete(int family, int sotype, int mapfd)
829+
static void test_syn_recv_insert_delete(struct test_sockmap_listen *skel __always_unused,
830+
int family, int sotype, int mapfd)
788831
{
789832
struct connect_accept_ctx ctx = { 0 };
790833
struct sockaddr_storage addr;
@@ -847,7 +890,8 @@ static void *listen_thread(void *arg)
847890
return NULL;
848891
}
849892

850-
static void test_race_insert_listen(int family, int socktype, int mapfd)
893+
static void test_race_insert_listen(struct test_sockmap_listen *skel __always_unused,
894+
int family, int socktype, int mapfd)
851895
{
852896
struct connect_accept_ctx ctx = { 0 };
853897
const u32 zero = 0;
@@ -1473,7 +1517,8 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map,
14731517
int family, int sotype)
14741518
{
14751519
const struct op_test {
1476-
void (*fn)(int family, int sotype, int mapfd);
1520+
void (*fn)(struct test_sockmap_listen *skel,
1521+
int family, int sotype, int mapfd);
14771522
const char *name;
14781523
int sotype;
14791524
} tests[] = {
@@ -1520,7 +1565,7 @@ static void test_ops(struct test_sockmap_listen *skel, struct bpf_map *map,
15201565
if (!test__start_subtest(s))
15211566
continue;
15221567

1523-
t->fn(family, sotype, map_fd);
1568+
t->fn(skel, family, sotype, map_fd);
15241569
test_ops_cleanup(map);
15251570
}
15261571
}

0 commit comments

Comments
 (0)