| Message ID | 20260128124410.429529-2-ralf@mandelbit.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel,ovpn,net,v2,1/3] ovpn: set sk_user_data before overriding callbacks | expand |
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 <ralf@mandelbit.com> > --- > 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. (your current patch would also avoid a UAF on ovpn_peer_stats_increment_tx, but you're solving that in the next patch)
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 <ralf@mandelbit.com> > > --- > > 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.
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 */
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 <ralf@mandelbit.com> --- 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(+)