* Re: [Security] Vulnerability in: Linux kernel, openvswitch zerocopy underflow
[not found] ` <CAJ0BgHeKoF7nnwA1_V0PSVETA+T9z9EX=OtSM4vMxQJv=nZAPg@mail.gmail.com>
@ 2026-07-06 14:21 ` Aaron Conole
0 siblings, 0 replies; only message in thread
From: Aaron Conole @ 2026-07-06 14:21 UTC (permalink / raw)
To: 侯敏熙
Cc: netdev, Ilya Maximets, Kyle Zeng, Eelco Chaudron,
Outbound Disclosures, security
侯敏熙 <houminxi@gmail.com> writes:
>> Just an FYI - this vulnerability is now public after Sashiko scanned
>> a proposed patch:
>
> Thanks for the heads-up, Aaron.
>
> My test_trunc() only exercises the non-GSO forwarding path (veth +
> ping), so neither the GSO segmentation underflow nor the POP_ETH +
> TRUNC combination is covered.
>
> Once a fix lands on net-next, I'll extend the TRUNC test to verify
> the new cutlen semantics.
No need. Let's keep it as is. We can look at doing complex 'fuzzing'
stuff in the future, but for now just giving a heads up.
> happy to help with testing if needed.
>
> Minxi
>
> Aaron Conole <aconole@redhat.com> 于2026年7月6日周一 08:05写道：
>
> Ilya Maximets <i.maximets@ovn.org> writes:
>
> > On 6/26/26 5:12 AM, Kyle Zeng wrote:
> >> Hi Ilya,
> >>
> >> I just tested the patch. It prevents underflow. However, it leads to a
> >> BUG_ON crash, the PoC and crash splash are attached.
> >>
> >> The patch changes `OVS_CB(skb)->cutlen` from "number of bytes to
> >> remove" to "number of bytes to preserve." That also changes the
> >> no-truncation sentinel from 0 to U32_MAX.
> >> Most of the action paths were updated to use U32_MAX, but the
> >> miss-upcall path was not. It still does `error = ovs_dp_upcall(dp,
> >> skb, key, &upcall, 0);` in `ovs_dp_process_packet()`.
> >
> > This is a good point, I missed that part.
>
> Just an FYI - this vulnerability is now public after Sashiko scanned a
> proposed patch:
>
> https://sashiko.dev/#/patchset/20260702074926.1174810-1-houminxi%40gmail.com
>
> > This is a pre-existing issue, but while looking at how TRUNC is handled,
> > there appears to be an integer underflow vulnerability...
>
> Minxi has been adding tests for different actions, and proposed a TRUNC
> action test. When reviewing it, I looked at the AI comments from the two
> different Sashiko instances and saw this. I've CC'd Minxi as well,
> since he proposed the selftest.
>
> Further discussion probably doesn't need to be on the security only
> list, and I think it is best if we go to the public mailing list.
>
> >>
> >> Under the new semantics, that 0 is no longer "no truncation" It means
> >> "preserve zero bytes". As a result, `queue_userspace_packet()`
> >> computes:
> >> `skb_len = min(skb->len, cutlen); /* becomes 0 */` and then: `hlen =
> >> skb_len; /* also becomes 0 */` before calling `skb_zerocopy(user_skb,
> >> skb, skb_len, hlen);`.
> >>
> >> I adapted your patch and the new patch is attached.
> >
> > See some comments below.
> >
> >> From 5b2b6f328b339a22c8a96bdacda4b62884640696 Mon Sep 17 00:00:00 2001
> >> From: Kyle Zeng <kylebot@openai.com>
> >> Date: Fri, 26 Jun 2026 02:48:14 +0000
> >> Subject: [PATCH] openvswitch: fix GSO userspace truncation underflow
> >>
> >> OVS_ACTION_ATTR_TRUNC currently stores a delta from the original skb
> >> length in OVS_CB(skb)->cutlen. When a later userspace action segments a
> >> GSO skb, queue_gso_packets() reuses that delta for each smaller segment.
> >> A segment can then reach queue_userspace_packet() with cutlen greater
> >> than skb->len, underflowing the length passed to skb_zerocopy().
> >>
> >> Store the maximum preserved length instead and derive the effective
> >> length from each current skb with min(). Use U32_MAX as the
> >> no-truncation sentinel so the value remains valid if skb geometry
> >> changes before a consumer handles it.
> >>
> >> Fixes: f2a4d086ed4c ("openvswitch: Add packet truncation support.")
> >> Cc: stable@vger.kernel.org
> >> Assisted-by: Codex:gpt-5.5
> >> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> >> ---
> >> net/openvswitch/actions.c | 20 ++++++++------------
> >> net/openvswitch/datapath.c | 19 +++++++++++--------
> >> net/openvswitch/datapath.h | 3 ++-
> >> net/openvswitch/vport.c | 2 +-
> >> 4 files changed, 22 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> >> index 140388a18ae0..596ad1be8b6b 100644
> >> --- a/net/openvswitch/actions.c
> >> +++ b/net/openvswitch/actions.c
> >> @@ -837,12 +837,9 @@ static void do_output(struct datapath *dp, struct sk_buff
> *skb, int out_port,
> >> u16 mru = OVS_CB(skb)->mru;
> >> u32 cutlen = OVS_CB(skb)->cutlen;
> >>
> >> - if (unlikely(cutlen > 0)) {
> >> - if (skb->len - cutlen > ovs_mac_header_len(key))
> >> - pskb_trim(skb, skb->len - cutlen);
> >> - else
> >> - pskb_trim(skb, ovs_mac_header_len(key));
> >> - }
> >> + if (unlikely(cutlen != U32_MAX))
> >
> > There is no need to trim if cutlen >= skb->len. Is there some problem with
> > the approach I had in my diff:
> >
> > + if (unlikely(cutlen < skb->len))
> > + pskb_trim(skb, max(cutlen, ovs_mac_header_len(key)));
> >
> > ?
> >
> >> + pskb_trim(skb, max(min(skb->len, cutlen),
> >> + ovs_mac_header_len(key)));
> >>
> >> if (likely(!mru ||
> >> (skb->len <= mru + vport->dev->hard_header_len))) {
> >> @@ -1234,7 +1231,7 @@ static void execute_psample(struct datapath *dp, struct
> sk_buff *skb,
> >>
> >> psample_group.net⚠️ = ovs_dp_get_net(dp);
> >> md.in_ifindex = OVS_CB(skb)->input_vport->dev->ifindex;
> >> - md.trunc_size = skb->len - OVS_CB(skb)->cutlen;
> >> + md.trunc_size = min(skb->len, OVS_CB(skb)->cutlen);
> >> md.rate_as_probability = 1;
> >>
> >> rate = OVS_CB(skb)->probability ? OVS_CB(skb)->probability : U32_MAX;
> >> @@ -1284,22 +1281,21 @@ static int do_execute_actions(struct datapath *dp, struct
> sk_buff *skb,
> >> clone = skb_clone(skb, GFP_ATOMIC);
> >> if (clone)
> >> do_output(dp, clone, port, key);
> >> - OVS_CB(skb)->cutlen = 0;
> >> + OVS_CB(skb)->cutlen = U32_MAX;
> >> break;
> >> }
> >>
> >> case OVS_ACTION_ATTR_TRUNC: {
> >> struct ovs_action_trunc *trunc = nla_data(a);
> >>
> >> - if (skb->len > trunc->max_len)
> >> - OVS_CB(skb)->cutlen = skb->len - trunc->max_len;
> >> + OVS_CB(skb)->cutlen = trunc->max_len;
> >> break;
> >> }
> >>
> >> case OVS_ACTION_ATTR_USERSPACE:
> >> output_userspace(dp, skb, key, a, attr,
> >> len, OVS_CB(skb)->cutlen);
> >> - OVS_CB(skb)->cutlen = 0;
> >> + OVS_CB(skb)->cutlen = U32_MAX;
> >> if (nla_is_last(a, rem)) {
> >> consume_skb(skb);
> >> return 0;
> >> @@ -1453,7 +1449,7 @@ static int do_execute_actions(struct datapath *dp, struct
> sk_buff *skb,
> >>
> >> case OVS_ACTION_ATTR_PSAMPLE:
> >> execute_psample(dp, skb, a);
> >> - OVS_CB(skb)->cutlen = 0;
> >> + OVS_CB(skb)->cutlen = U32_MAX;
> >> if (nla_is_last(a, rem)) {
> >> consume_skb(skb);
> >> return 0;
> >> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> >> index f0164817d9b7..66c621a611b7 100644
> >> --- a/net/openvswitch/datapath.c
> >> +++ b/net/openvswitch/datapath.c
> >> @@ -276,7 +276,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct
> sw_flow_key *key)
> >> upcall.portid = ovs_vport_find_upcall_portid(p, skb);
> >>
> >> upcall.mru = OVS_CB(skb)->mru;
> >> - error = ovs_dp_upcall(dp, skb, key, &upcall, 0);
> >> + error = ovs_dp_upcall(dp, skb, key, &upcall, U32_MAX);
> >> switch (error) {
> >> case 0:
> >> case -EAGAIN:
> >> @@ -458,6 +458,7 @@ static int queue_userspace_packet(struct datapath *dp, struct
> sk_buff *skb,
> >> struct sk_buff *user_skb = NULL; /* to be queued to userspace */
> >> struct nlattr *nla;
> >> size_t len;
> >
> > I would still rename this into msg_size. Having 'len' and 'skb_len'
> > in the same scope is confusing.
> >
> >> + unsigned int skb_len;
> >> unsigned int hlen;
> >> int err, dp_ifindex;
> >> u64 hash;
> >> @@ -478,7 +479,8 @@ static int queue_userspace_packet(struct datapath *dp, struct
> sk_buff *skb,
> >> skb = nskb;
> >> }
> >>
> >> - if (nla_attr_size(skb->len) > USHRT_MAX) {
> >> + skb_len = min(skb->len, cutlen);
> >> + if (nla_attr_size(skb_len) > USHRT_MAX) {
> >> err = -EFBIG;
> >> goto out;
> >> }
> >> @@ -493,11 +495,11 @@ static int queue_userspace_packet(struct datapath *dp,
> struct sk_buff *skb,
> >> * padding logic. Only perform zerocopy if padding is not required.
> >> */
> >> if (dp->user_features & OVS_DP_F_UNALIGNED)
> >> - hlen = skb_zerocopy_headlen(skb);
> >> + hlen = min(skb_zerocopy_headlen(skb), skb_len);
> >> else
> >> - hlen = skb->len;
> >> + hlen = skb_len;
> >>
> >> - len = upcall_msg_size(upcall_info, hlen - cutlen,
> >> + len = upcall_msg_size(upcall_info, hlen,
> >> OVS_CB(skb)->acts_origlen);
> >> user_skb = genlmsg_new(len, GFP_ATOMIC);
> >> if (!user_skb) {
> >> @@ -560,7 +562,7 @@ static int queue_userspace_packet(struct datapath *dp, struct
> sk_buff *skb,
> >> }
> >>
> >> /* Add OVS_PACKET_ATTR_LEN when packet is truncated */
> >> - if (cutlen > 0 &&
> >> + if (cutlen != U32_MAX &&
> >
> > This changes the logic. The attribute should be included only if the
> > packet was actually truncated, i.e., if skb_len < skb->len.
> >
> >> nla_put_u32(user_skb, OVS_PACKET_ATTR_LEN, skb->len)) {
> >> err = -ENOBUFS;
> >> goto out;
> >> @@ -585,9 +587,9 @@ static int queue_userspace_packet(struct datapath *dp, struct
> sk_buff *skb,
> >> err = -ENOBUFS;
> >> goto out;
> >> }
> >> - nla->nla_len = nla_attr_size(skb->len - cutlen);
> >> + nla->nla_len = nla_attr_size(skb_len);
> >>
> >> - err = skb_zerocopy(user_skb, skb, skb->len - cutlen, hlen);
> >> + err = skb_zerocopy(user_skb, skb, skb_len, hlen);
> >> if (err)
> >> goto out;
> >>
> >> @@ -644,6 +646,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct
> genl_info *info)
> >> packet->ignore_df = 1;
> >> }
> >> OVS_CB(packet)->mru = mru;
> >> + OVS_CB(packet)->cutlen = U32_MAX;
> >>
> >> if (a[OVS_PACKET_ATTR_HASH]) {
> >> hash = nla_get_u64(a[OVS_PACKET_ATTR_HASH]);
> >> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> >> index db0c3e69d66c..11fa104b6172 100644
> >> --- a/net/openvswitch/datapath.h
> >> +++ b/net/openvswitch/datapath.h
> >> @@ -118,7 +118,8 @@ struct datapath {
> >> * @mru: The maximum received fragement size; 0 if the packet is not
> >> * fragmented.
> >> * @acts_origlen: The netlink size of the flow actions applied to this skb.
> >> - * @cutlen: The number of bytes from the packet end to be removed.
> >> + * @cutlen: The maximum number of bytes to preserve on output. U32_MAX means
> >> + * no truncation.
> >> * @probability: The sampling probability that was applied to this skb; 0 means
> >> * no sampling has occurred; U32_MAX means 100% probability.
> >> * @upcall_pid: Netlink socket PID to use for sending this packet to userspace;
> >> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> >> index 56b2e2d1a749..12741485c939 100644
> >> --- a/net/openvswitch/vport.c
> >> +++ b/net/openvswitch/vport.c
> >> @@ -502,7 +502,7 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
> >>
> >> OVS_CB(skb)->input_vport = vport;
> >> OVS_CB(skb)->mru = 0;
> >> - OVS_CB(skb)->cutlen = 0;
> >> + OVS_CB(skb)->cutlen = U32_MAX;
> >> OVS_CB(skb)->probability = 0;
> >> OVS_CB(skb)->upcall_pid = 0;
> >> if (unlikely(dev_net(skb->dev) != ovs_dp_get_net(vport->dp))) {
> >> --
> >> 2.54.0
> >>
^ permalink raw reply [flat|nested] only message in thread