On Thu, 2026-01-29 at 12:16 +0100, Sabrina Dubroca wrote:
> 2026-01-28, 13:44:09 +0100, Ralf Lici wrote:
> > During GSO fragmentation, skb_share_check may clone the first
> > segment
> > and free the original skb. The current implementation continues to
> > use
> > the stale skb pointer for peer lookup.
> >
> > Fix this by updating the skb variable to point to the new head of
> > the
> > segment list after the processing loop. Additionally, return early
> > if
> > all segments were dropped during the loop to avoid double-counting
> > statistics and double-freeing memory in the drop path.
> >
> > Fixes: 08857b5ec5d9 ("ovpn: implement basic TX path (UDP)")
> > Signed-off-by: Ralf Lici <[email protected]>
> > ---
> > Changes since v1
> > - this is a new patch that replaces the previous "ovpn: use
> > sk_buff_head
> > properly in ovpn_net_xmit"
> >
> > drivers/net/ovpn/io.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
> > index 3e9e7f8444b3..95c3518e067c 100644
> > --- a/drivers/net/ovpn/io.c
> > +++ b/drivers/net/ovpn/io.c
> > @@ -396,6 +396,17 @@ netdev_tx_t ovpn_net_xmit(struct sk_buff *skb,
> > struct net_device *dev)
> >
> > __skb_queue_tail(&skb_list, curr);
> > }
> > +
> > + /* no segments survived: don't jump to 'drop' because we
> > already
> > + * incremented the counter for each failure in the loop
> > + */
> > + if (unlikely(skb_queue_empty(&skb_list)))
> > + return NETDEV_TX_OK;
> > +
> > + /* the original 'skb' might have been freed/cloned in the
> > loop: use the
> > + * first element of our list for the other operations
> > + */
> > + skb = skb_list.next;
> > skb_list.prev->next = NULL;
> >
> > /* retrieve peer serving the destination IP of this packet
> > */
>
> This patch looks ok, but maybe it would be better to fix this using a
> chunk of your sk_buff_head rework instead. I think all segments
> produced by skb_gso_segment will have a dst attached
> (__copy_skb_header(nskb, head_skb) in skb_segment), so the
> skb_dst_drop after peer selection will only affect the first segment,
> and not the others. Doing peer selection + skb_dst_drop before calling
> skb_gso_segment would solve this as well as the UAF.
Ah right, the dst is indeed copied to the segments. I will make sure to
move the peer selection earlier, while we are still using the original
skb, so that we can call skb_dst_drop only on that one.
> (your current patch would also avoid a UAF on
> ovpn_peer_stats_increment_tx,
> but you're solving that in the next patch)
Yes, now the stats patch is only responsible for fixing the stats count.
I have also updated the commit message from v1.
Thanks for the feedback.
--
Ralf Lici
Mandelbit Srl
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
