| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z3uzXOzV1T3Uv4f+@gauss3.secunet.de>
Date: Mon, 6 Jan 2025 11:41:32 +0100
From: Steffen Klassert <steffen.klassert@...unet.com>
To: Eric Dumazet <edumazet@...gle.com>
CC: Shahar Shitrit <shshitrit@...dia.com>, "brianvv@...gle.com"
<brianvv@...gle.com>, "davem@...emloft.net" <davem@...emloft.net>,
"eric.dumazet@...il.com" <eric.dumazet@...il.com>, "kuba@...nel.org"
<kuba@...nel.org>, "kuniyu@...zon.com" <kuniyu@...zon.com>,
"martin.lau@...nel.org" <martin.lau@...nel.org>, "ncardwell@...gle.com"
<ncardwell@...gle.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>, Tariq Toukan <tariqt@...dia.com>,
Gal Pressman <gal@...dia.com>, Ziyad Atiyyeh <ziyadat@...dia.com>, "Dror
Tennenbaum" <drort@...dia.com>
Subject: Re: [PATCH v3 net-next 4/5] ipv6: tcp: give socket pointer to
control skbs
On Wed, Dec 18, 2024 at 10:04:19AM +0100, Eric Dumazet wrote:
> On Wed, Dec 18, 2024 at 9:26 AM Steffen Klassert
> <steffen.klassert@...unet.com> wrote:
> >
> > Can you provide a bit more context? I don't see the problem.
>
>
> skb->sk is used in xfrm, and the assumption is that it is a full socket.
>
> List of things that are supported, even for timewait and
> request_sockets, and used in xfrm:
>
> sk->sk_bound_dev_if
> sock_net(skb->sk)
> inet_sk(sk)->inet_dport;
>
> But xfrmi_xmit2() for instance is doing :
>
> err = dst_output(xi->net, skb->sk, skb);
>
> Other dst_output() users use : dst_output(net, sk, skb), because sk
> can be different than skb->sk
>
> Also xfrm6_local_dontfrag() is assuming sk is an inet6 socket:
>
> if (proto == IPPROTO_UDP || proto == IPPROTO_RAW)
> return inet6_test_bit(DONTFRAG, sk);
>
> xfrm_lookup_with_ifid() seems ok (it uses sk_const_to_full_sk()), but
> we probably miss some fixes.
Thanks for the explanation!
I did a first audit of the xfrm stack to find the problematic
skb->sk usages. The result is the (still untested) patch below.
I need to go over it again to make sure I found everyting.
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index f3281312eb5e..8cf5f6634775 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -279,7 +279,7 @@ static void esp_output_done(void *data, int err)
x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP)
esp_output_tail_tcp(x, skb);
else
- xfrm_output_resume(skb->sk, skb, err);
+ xfrm_output_resume(skb_to_full_sk(skb), skb, err);
}
}
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index b2400c226a32..fad4d7c9fa50 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -315,7 +315,7 @@ static void esp_output_done(void *data, int err)
x->encap && x->encap->encap_type == TCP_ENCAP_ESPINTCP)
esp_output_tail_tcp(x, skb);
else
- xfrm_output_resume(skb->sk, skb, err);
+ xfrm_output_resume(skb_to_full_sk(skb), skb, err);
}
}
diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
index 5f7b1fdbffe6..b3d5d1f266ee 100644
--- a/net/ipv6/xfrm6_output.c
+++ b/net/ipv6/xfrm6_output.c
@@ -82,14 +82,14 @@ static int __xfrm6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
toobig = skb->len > mtu && !skb_is_gso(skb);
- if (toobig && xfrm6_local_dontfrag(skb->sk)) {
+ if (toobig && xfrm6_local_dontfrag(sk)) {
xfrm6_local_rxpmtu(skb, mtu);
kfree_skb(skb);
return -EMSGSIZE;
} else if (toobig && xfrm6_noneed_fragment(skb)) {
skb->ignore_df = 1;
goto skip_frag;
- } else if (!skb->ignore_df && toobig && skb->sk) {
+ } else if (!skb->ignore_df && toobig && sk) {
xfrm_local_error(skb, mtu);
kfree_skb(skb);
return -EMSGSIZE;
diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c
index 98f1e2b67c76..c397eb99d867 100644
--- a/net/xfrm/xfrm_interface_core.c
+++ b/net/xfrm/xfrm_interface_core.c
@@ -506,7 +506,7 @@ xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
skb_dst_set(skb, dst);
skb->dev = tdev;
- err = dst_output(xi->net, skb->sk, skb);
+ err = dst_output(xi->net, skb_to_full_sk(skb), skb);
if (net_xmit_eval(err) == 0) {
dev_sw_netstats_tx_add(dev, 1, length);
} else {
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index e5722c95b8bb..1fb4dc0a76e1 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -796,7 +796,7 @@ static int xfrm4_tunnel_check_size(struct sk_buff *skb)
!skb_gso_validate_network_len(skb, ip_skb_dst_mtu(skb->sk, skb)))) {
skb->protocol = htons(ETH_P_IP);
- if (skb->sk)
+ if (skb->sk && sk_fullsock(skb->sk))
xfrm_local_error(skb, mtu);
else
icmp_send(skb, ICMP_DEST_UNREACH,
@@ -832,6 +832,7 @@ static int xfrm6_tunnel_check_size(struct sk_buff *skb)
{
int mtu, ret = 0;
struct dst_entry *dst = skb_dst(skb);
+ struct sock *sk = skb_to_full_sk(skb);
if (skb->ignore_df)
goto out;
@@ -846,9 +847,9 @@ static int xfrm6_tunnel_check_size(struct sk_buff *skb)
skb->dev = dst->dev;
skb->protocol = htons(ETH_P_IPV6);
- if (xfrm6_local_dontfrag(skb->sk))
+ if (xfrm6_local_dontfrag(sk))
ipv6_stub->xfrm6_local_rxpmtu(skb, mtu);
- else if (skb->sk)
+ else if (sk)
xfrm_local_error(skb, mtu);
else
icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, mtu);
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4408c11c0835..c27da1fd070e 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2959,7 +2959,7 @@ static void xfrm_policy_queue_process(struct timer_list *t)
skb_dst_drop(skb);
skb_dst_set(skb, dst);
- dst_output(net, skb->sk, skb);
+ dst_output(net, skb_to_full_sk(skb), skb);
}
out:
Powered by blists - more mailing lists