Skip to content

Commit b604eb8

Browse files
Eric Dumazetgregkh
Eric Dumazet
authored and
gregkh
committed
tcp: add tcp_v4_fill_cb()/tcp_v4_restore_cb()
[ Upstream commit eeea10b ] James Morris reported kernel stack corruption bug [1] while running the SELinux testsuite, and bisected to a recent commit bffa72c ("net: sk_buff rbnode reorg") We believe this commit is fine, but exposes an older bug. SELinux code runs from tcp_filter() and might send an ICMP, expecting IP options to be found in skb->cb[] using regular IPCB placement. We need to defer TCP mangling of skb->cb[] after tcp_filter() calls. This patch adds tcp_v4_fill_cb()/tcp_v4_restore_cb() in a very similar way we added them for IPv6. [1] [ 339.806024] SELinux: failure in selinux_parse_skb(), unable to parse packet [ 339.822505] Kernel panic - not syncing: stack-protector: Kernel stack is corrupted in: ffffffff81745af5 [ 339.822505] [ 339.852250] CPU: 4 PID: 3642 Comm: client Not tainted 4.15.0-rc1-test #15 [ 339.868498] Hardware name: LENOVO 10FGS0VA1L/30BC, BIOS FWKT68A 01/19/2017 [ 339.885060] Call Trace: [ 339.896875] <IRQ> [ 339.908103] dump_stack+0x63/0x87 [ 339.920645] panic+0xe8/0x248 [ 339.932668] ? ip_push_pending_frames+0x33/0x40 [ 339.946328] ? icmp_send+0x525/0x530 [ 339.958861] ? kfree_skbmem+0x60/0x70 [ 339.971431] __stack_chk_fail+0x1b/0x20 [ 339.984049] icmp_send+0x525/0x530 [ 339.996205] ? netlbl_skbuff_err+0x36/0x40 [ 340.008997] ? selinux_netlbl_err+0x11/0x20 [ 340.021816] ? selinux_socket_sock_rcv_skb+0x211/0x230 [ 340.035529] ? security_sock_rcv_skb+0x3b/0x50 [ 340.048471] ? sk_filter_trim_cap+0x44/0x1c0 [ 340.061246] ? tcp_v4_inbound_md5_hash+0x69/0x1b0 [ 340.074562] ? tcp_filter+0x2c/0x40 [ 340.086400] ? tcp_v4_rcv+0x820/0xa20 [ 340.098329] ? ip_local_deliver_finish+0x71/0x1a0 [ 340.111279] ? ip_local_deliver+0x6f/0xe0 [ 340.123535] ? ip_rcv_finish+0x3a0/0x3a0 [ 340.135523] ? ip_rcv_finish+0xdb/0x3a0 [ 340.147442] ? ip_rcv+0x27c/0x3c0 [ 340.158668] ? inet_del_offload+0x40/0x40 [ 340.170580] ? __netif_receive_skb_core+0x4ac/0x900 [ 340.183285] ? rcu_accelerate_cbs+0x5b/0x80 [ 340.195282] ? __netif_receive_skb+0x18/0x60 [ 340.207288] ? process_backlog+0x95/0x140 [ 340.218948] ? net_rx_action+0x26c/0x3b0 [ 340.230416] ? __do_softirq+0xc9/0x26a [ 340.241625] ? do_softirq_own_stack+0x2a/0x40 [ 340.253368] </IRQ> [ 340.262673] ? do_softirq+0x50/0x60 [ 340.273450] ? __local_bh_enable_ip+0x57/0x60 [ 340.285045] ? ip_finish_output2+0x175/0x350 [ 340.296403] ? ip_finish_output+0x127/0x1d0 [ 340.307665] ? nf_hook_slow+0x3c/0xb0 [ 340.318230] ? ip_output+0x72/0xe0 [ 340.328524] ? ip_fragment.constprop.54+0x80/0x80 [ 340.340070] ? ip_local_out+0x35/0x40 [ 340.350497] ? ip_queue_xmit+0x15c/0x3f0 [ 340.361060] ? __kmalloc_reserve.isra.40+0x31/0x90 [ 340.372484] ? __skb_clone+0x2e/0x130 [ 340.382633] ? tcp_transmit_skb+0x558/0xa10 [ 340.393262] ? tcp_connect+0x938/0xad0 [ 340.403370] ? ktime_get_with_offset+0x4c/0xb0 [ 340.414206] ? tcp_v4_connect+0x457/0x4e0 [ 340.424471] ? __inet_stream_connect+0xb3/0x300 [ 340.435195] ? inet_stream_connect+0x3b/0x60 [ 340.445607] ? SYSC_connect+0xd9/0x110 [ 340.455455] ? __audit_syscall_entry+0xaf/0x100 [ 340.466112] ? syscall_trace_enter+0x1d0/0x2b0 [ 340.476636] ? __audit_syscall_exit+0x209/0x290 [ 340.487151] ? SyS_connect+0xe/0x10 [ 340.496453] ? do_syscall_64+0x67/0x1b0 [ 340.506078] ? entry_SYSCALL64_slow_path+0x25/0x25 Fixes: 971f10e ("tcp: better TCP_SKB_CB layout to reduce cache line misses") Signed-off-by: Eric Dumazet <[email protected]> Reported-by: James Morris <[email protected]> Tested-by: James Morris <[email protected]> Tested-by: Casey Schaufler <[email protected]> Signed-off-by: David S. Miller <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent dacf127 commit b604eb8

File tree

2 files changed

+46
-23
lines changed

2 files changed

+46
-23
lines changed

‎net/ipv4/tcp_ipv4.c

Lines changed: 40 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1587,6 +1587,34 @@ int tcp_filter(struct sock *sk, struct sk_buff *skb)
15871587
}
15881588
EXPORT_SYMBOL(tcp_filter);
15891589

1590+
static void tcp_v4_restore_cb(struct sk_buff *skb)
1591+
{
1592+
memmove(IPCB(skb), &TCP_SKB_CB(skb)->header.h4,
1593+
sizeof(struct inet_skb_parm));
1594+
}
1595+
1596+
static void tcp_v4_fill_cb(struct sk_buff *skb, const struct iphdr *iph,
1597+
const struct tcphdr *th)
1598+
{
1599+
/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
1600+
* barrier() makes sure compiler wont play fool^Waliasing games.
1601+
*/
1602+
memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
1603+
sizeof(struct inet_skb_parm));
1604+
barrier();
1605+
1606+
TCP_SKB_CB(skb)->seq = ntohl(th->seq);
1607+
TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
1608+
skb->len - th->doff * 4);
1609+
TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
1610+
TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
1611+
TCP_SKB_CB(skb)->tcp_tw_isn = 0;
1612+
TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
1613+
TCP_SKB_CB(skb)->sacked = 0;
1614+
TCP_SKB_CB(skb)->has_rxtstamp =
1615+
skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
1616+
}
1617+
15901618
/*
15911619
* From tcp_input.c
15921620
*/
@@ -1627,24 +1655,6 @@ int tcp_v4_rcv(struct sk_buff *skb)
16271655

16281656
th = (const struct tcphdr *)skb->data;
16291657
iph = ip_hdr(skb);
1630-
/* This is tricky : We move IPCB at its correct location into TCP_SKB_CB()
1631-
* barrier() makes sure compiler wont play fool^Waliasing games.
1632-
*/
1633-
memmove(&TCP_SKB_CB(skb)->header.h4, IPCB(skb),
1634-
sizeof(struct inet_skb_parm));
1635-
barrier();
1636-
1637-
TCP_SKB_CB(skb)->seq = ntohl(th->seq);
1638-
TCP_SKB_CB(skb)->end_seq = (TCP_SKB_CB(skb)->seq + th->syn + th->fin +
1639-
skb->len - th->doff * 4);
1640-
TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
1641-
TCP_SKB_CB(skb)->tcp_flags = tcp_flag_byte(th);
1642-
TCP_SKB_CB(skb)->tcp_tw_isn = 0;
1643-
TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
1644-
TCP_SKB_CB(skb)->sacked = 0;
1645-
TCP_SKB_CB(skb)->has_rxtstamp =
1646-
skb->tstamp || skb_hwtstamps(skb)->hwtstamp;
1647-
16481658
lookup:
16491659
sk = __inet_lookup_skb(&tcp_hashinfo, skb, __tcp_hdrlen(th), th->source,
16501660
th->dest, sdif, &refcounted);
@@ -1675,14 +1685,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
16751685
sock_hold(sk);
16761686
refcounted = true;
16771687
nsk = NULL;
1678-
if (!tcp_filter(sk, skb))
1688+
if (!tcp_filter(sk, skb)) {
1689+
th = (const struct tcphdr *)skb->data;
1690+
iph = ip_hdr(skb);
1691+
tcp_v4_fill_cb(skb, iph, th);
16791692
nsk = tcp_check_req(sk, skb, req, false);
1693+
}
16801694
if (!nsk) {
16811695
reqsk_put(req);
16821696
goto discard_and_relse;
16831697
}
16841698
if (nsk == sk) {
16851699
reqsk_put(req);
1700+
tcp_v4_restore_cb(skb);
16861701
} else if (tcp_child_process(sk, nsk, skb)) {
16871702
tcp_v4_send_reset(nsk, skb);
16881703
goto discard_and_relse;
@@ -1708,6 +1723,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
17081723
goto discard_and_relse;
17091724
th = (const struct tcphdr *)skb->data;
17101725
iph = ip_hdr(skb);
1726+
tcp_v4_fill_cb(skb, iph, th);
17111727

17121728
skb->dev = NULL;
17131729

@@ -1738,6 +1754,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
17381754
if (!xfrm4_policy_check(NULL, XFRM_POLICY_IN, skb))
17391755
goto discard_it;
17401756

1757+
tcp_v4_fill_cb(skb, iph, th);
1758+
17411759
if (tcp_checksum_complete(skb)) {
17421760
csum_error:
17431761
__TCP_INC_STATS(net, TCP_MIB_CSUMERRORS);
@@ -1764,6 +1782,8 @@ int tcp_v4_rcv(struct sk_buff *skb)
17641782
goto discard_it;
17651783
}
17661784

1785+
tcp_v4_fill_cb(skb, iph, th);
1786+
17671787
if (tcp_checksum_complete(skb)) {
17681788
inet_twsk_put(inet_twsk(sk));
17691789
goto csum_error;
@@ -1780,6 +1800,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
17801800
if (sk2) {
17811801
inet_twsk_deschedule_put(inet_twsk(sk));
17821802
sk = sk2;
1803+
tcp_v4_restore_cb(skb);
17831804
refcounted = false;
17841805
goto process;
17851806
}

‎net/ipv6/tcp_ipv6.c

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1448,7 +1448,6 @@ static int tcp_v6_rcv(struct sk_buff *skb)
14481448
struct sock *nsk;
14491449

14501450
sk = req->rsk_listener;
1451-
tcp_v6_fill_cb(skb, hdr, th);
14521451
if (tcp_v6_inbound_md5_hash(sk, skb)) {
14531452
sk_drops_add(sk, skb);
14541453
reqsk_put(req);
@@ -1461,8 +1460,12 @@ static int tcp_v6_rcv(struct sk_buff *skb)
14611460
sock_hold(sk);
14621461
refcounted = true;
14631462
nsk = NULL;
1464-
if (!tcp_filter(sk, skb))
1463+
if (!tcp_filter(sk, skb)) {
1464+
th = (const struct tcphdr *)skb->data;
1465+
hdr = ipv6_hdr(skb);
1466+
tcp_v6_fill_cb(skb, hdr, th);
14651467
nsk = tcp_check_req(sk, skb, req, false);
1468+
}
14661469
if (!nsk) {
14671470
reqsk_put(req);
14681471
goto discard_and_relse;
@@ -1486,15 +1489,14 @@ static int tcp_v6_rcv(struct sk_buff *skb)
14861489
if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
14871490
goto discard_and_relse;
14881491

1489-
tcp_v6_fill_cb(skb, hdr, th);
1490-
14911492
if (tcp_v6_inbound_md5_hash(sk, skb))
14921493
goto discard_and_relse;
14931494

14941495
if (tcp_filter(sk, skb))
14951496
goto discard_and_relse;
14961497
th = (const struct tcphdr *)skb->data;
14971498
hdr = ipv6_hdr(skb);
1499+
tcp_v6_fill_cb(skb, hdr, th);
14981500

14991501
skb->dev = NULL;
15001502

0 commit comments

Comments
 (0)