| 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: <IA1PR11MB628988D4CC8F4E9B1903B37589C52@IA1PR11MB6289.namprd11.prod.outlook.com>
Date: Wed, 19 Feb 2025 15:31:32 +0000
From: "Joshi, Sreedevi" <sreedevi.joshi@...el.com>
To: sreedevi.joshi <joshisre@...mtp.an.intel.com>, "edumazet@...il.com"
<edumazet@...il.com>, "kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>, "horms@...nel.org"
<horms@...nel.org>, "ast@...nel.org" <ast@...nel.org>, "daniel@...earbox.net"
<daniel@...earbox.net>
CC: "Karlsson, Magnus" <magnus.karlsson@...el.com>, "Fijalkowski, Maciej"
<maciej.fijalkowski@...el.com>, "hawk@...nel.org" <hawk@...nel.org>,
"john.fastabend@...il.com" <john.fastabend@...il.com>,
"almasrymina@...gle.com" <almasrymina@...gle.com>, "asml.silence@...il.com"
<asml.silence@...il.com>, "lorenzo@...nel.org" <lorenzo@...nel.org>,
"Lobakin, Aleksander" <aleksander.lobakin@...el.com>, "chopps@...n.net"
<chopps@...n.net>, "bigeasy@...utronix.de" <bigeasy@...utronix.de>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: RE: [RFC PATCH net 0/1] transport_header set incorrectly when using
veth
> -----Original Message-----
> From: Joshi, Sreedevi
> Sent: Friday, February 7, 2025 9:28 AM
> To: sreedevi.joshi <joshisre@...mtp.an.intel.com>; edumazet@...il.com; kuba@...nel.org; pabeni@...hat.com; horms@...nel.org;
> ast@...nel.org; daniel@...earbox.net
> Cc: Karlsson, Magnus <magnus.karlsson@...el.com>; Fijalkowski, Maciej <maciej.fijalkowski@...el.com>; hawk@...nel.org;
> john.fastabend@...il.com; almasrymina@...gle.com; asml.silence@...il.com; lorenzo@...nel.org; Lobakin, Aleksander
> <aleksander.lobakin@...el.com>; chopps@...n.net; bigeasy@...utronix.de; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> bpf@...r.kernel.org
> Subject: RE: [RFC PATCH net 0/1] transport_header set incorrectly when using veth
>
>
> > -----Original Message-----
> > From: sreedevi.joshi <joshisre@...mtp.an.intel.com>
> > Sent: Thursday, February 6, 2025 1:06 PM
> > To: edumazet@...il.com; kuba@...nel.org; pabeni@...hat.com; horms@...nel.org; ast@...nel.org; daniel@...earbox.net
> > Cc: Karlsson, Magnus <magnus.karlsson@...el.com>; Fijalkowski, Maciej <maciej.fijalkowski@...el.com>; hawk@...nel.org;
> > john.fastabend@...il.com; almasrymina@...gle.com; asml.silence@...il.com; lorenzo@...nel.org; Lobakin, Aleksander
> > <aleksander.lobakin@...el.com>; chopps@...n.net; bigeasy@...utronix.de; netdev@...r.kernel.org; linux-kernel@...r.kernel.org;
> > bpf@...r.kernel.org; Joshi, Sreedevi <sreedevi.joshi@...el.com>
> > Subject: [RFC PATCH net 0/1] transport_header set incorrectly when using veth
> >
> > From: Sreedevi Joshi <sreedevi.joshi@...el.com>
> >
> > When testing a use-case on veth by attaching XDP and tc ingress hooks, it was noticed that the transport_header is set incorrectly and
> > causes the tc_ingress hook that is using bpf_skb_change_tail() call to report a failure.
> >
> > Here is the flow:
> > veth ingress:
> > veth_convert_skb_to_xdp_buff()- [Example: skb->trannsport_header=65535 skb->network_header=0]
> > ..>skb_pp_cow_data()
> > ....>skb_headers_offset_update() - adds offset without checking and this
> > results in transport_header value roll over.
> > [off: 192: results in skb->transport_header = 191, skb->network_header=192] tc_ingress hook: bpf_skb_change_tail()
> > - Since transport_header < network_header, min_len is negative and it fails.
> >
> > Two possbible solutions:
> > option 1: introducing the check in the skb_headers_offset_update() to skip adding offset
> > to transport_header when it is not set. (patch attached) option 2: reset transport header in veth_xdp_rcv_skb()
> >
> > Option 1 seems to be better as it will apply to any other interfaces that may use skb_headers_offset_update and there seems to
> > similar logic in the same function to check if mac_header was set before adding offset.
> >
> > Seeking your inputs on this.
> >
> > NOTES:
> > 1. If veth is used without XDP hook attached, this issue is not observed as the logic uses __netif_rx() directly and the transport header
> > is reset in __netif_receive_skb_core() as it detects it is not set.
> >
> > 2. Tested on i40e driver and confirmed it does not have this issue as the
> > skb_headers_offset_update() is not in the processing path.
> >
> >
> > Instructions to reproduce the issue along with the XDP and tc ingress programs is attached below.
> >
> > -------------------------------8<-------------------------------
> > instructions:
> >
> > #build XDP and tc programs
> > clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c xdp_prog.c -o xdp_prog.o clang -O2 -g -target bpf -D__TARGET_ARCH_x86 -c
> > tc_bpf_prog.c -o tc_bpf_prog.o
> >
> > # create the veth pair
> > ip link add veth0 numtxqueues 1 numrxqueues 1 type veth peer name veth1 \
> > numtxqueues 1 numrxqueues 1
> >
> > ip addr add 10.0.1.0/24 dev veth0
> > ip addr add 10.0.1.1/24 dev veth1
> > ip link set veth0 address 02:00:00:00:00:00 ip link set veth1 address 02:00:00:00:00:01 ip link set veth0 up ip link set veth1 up
> >
> > if [ -f /proc/net/if_inet6 ]; then
> > echo 1 > /proc/sys/net/ipv6/conf/veth0/disable_ipv6
> > echo 1 > /proc/sys/net/ipv6/conf/veth1/disable_ipv6
> > fi
> >
> > #attach xdp hook and tc ingress hooks to veth1 xdp-loader load veth1 xdp_prog.o
> >
> > tc qdisc add dev veth1 clsact
> > tc filter add dev veth1 ingress bpf da obj tc_bpf_prog.o sec prog
> >
> > # generate traffic from veth0 egress -> veth1 ingress ping -c e 10.0.1.3 -I veth0
> >
> > # observe the trace pipe (make sure tracing is on) # The following prints will appear
> > # ping-5330 [072] ..s2. 18266.403464: bpf_trace_printk: Failure.. new len=52 ret=-22
> > cat /sys/kernel/debug/tracing/trace_pipe
> >
> > -------------------------------8<-------------------------------
> > xdp_prog.c:
> >
> > #include <linux/bpf.h>
> > #include <bpf/bpf_helpers.h>
> >
> > SEC("xdp") int netd_xdp_prog(struct xdp_md *xdp) {
> > /* Squash compiler warning. */
> > (void)xdp;
> >
> > return XDP_PASS;
> > }
> >
> > char _license[] SEC("license") = "GPL";
> >
> > -------------------------------8<-------------------------------
> > test_bpf_prog.c:
> >
> > #include <linux/bpf.h>
> > #include <bpf/bpf_helpers.h>
> > #include <linux/pkt_cls.h>
> >
> > SEC("prog") int netd_tc_test_ingress(struct __sk_buff *skb)
> > {
> > long ret;
> >
> > /* extend skb length by 10 */
> > ret = bpf_skb_change_tail(skb, skb->len + 10, 0);
> > if (ret < 0) {
> > bpf_printk("Failure.. new len=%d ret=%d\n", skb->len+10, ret);
> > return TC_ACT_SHOT;
> > }
> >
> > bpf_printk("Success new len:%d \n", skb->len+10);
> >
> > return TC_ACT_UNSPEC;
> > }
> >
> > char _license[] SEC("license") = "GPL";
> >
> > -------------------------------8<-------------------------------
> >
> > Sreedevi Joshi (1):
> > net: check transport_header before adding offset
> >
> > net/core/skbuff.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > --
> > 2.25.1
> []
> Apologies for resending. Mail server had some issues earlier and didn't reach some recipients.
The following commit that was introduced in 6.12.8 addresses this issue. We no longer
need to follow up on this RFC.
Thanks for your time!
Sreedevi
commit 9ecc4d858b92c1bb0673ad9c327298e600c55659
Author: Cong Wang <cong.wang@...edance.com>
Date: Thu Dec 12 19:40:54 2024 -0800
bpf: Check negative offsets in __bpf_skb_min_len()
skb_network_offset() and skb_transport_offset() can be negative when
they are called after we pull the transport header, for example, when
we use eBPF sockmap at the point of ->sk_data_ready().
__bpf_skb_min_len() uses an unsigned int to get these offsets, this
leads to a very large number which then causes bpf_skb_change_tail()
failed unexpectedly.
Fix this by using a signed int to get these offsets and ensure the
minimum is at least zero.
Fixes: 5293efe62df8 ("bpf: add bpf_skb_change_tail helper")
Signed-off-by: Cong Wang <cong.wang@...edance.com>
Signed-off-by: Daniel Borkmann <daniel@...earbox.net>
Acked-by: John Fastabend <john.fastabend@...il.com>
Link: https://lore.kernel.org/bpf/20241213034057.246437-2-xiyou.wangcong@gmail.com
Powered by blists - more mailing lists