| Message ID | ZK9ZiNMsJX8+1F3N@debian.debian (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] gso: fix GSO_DODGY bit handling for related protocols | expand |
On Wed, Jul 12, 2023 at 9:55 PM Yan Zhai <yan@cloudflare.com> wrote: > > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source. > The canonical way is to recompute the gso_segs to avoid device driver > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the > egress of later devices, e.g. packets can egress to a vlan device backed > by a real NIC. > > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 > packets.") checks DODGY bit for UDP, but for packets that can be fed > directly to the device after gso_segs reset, it actually falls through > to fragmentation [1]. > > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417 > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY > bit after recomputing gso_segs. > > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit > at other places. These two things should not be conflated. Only the USO fix is strictly needed to fix the reported issue. > Fixes: 90017accff61 ("sctp: Add GSO support") > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") Link: https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > --- > [1]: > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > --- > net/ipv4/tcp_offload.c | 1 + > net/ipv4/udp_offload.c | 19 +++++++++++++++---- > net/ipv6/udp_offload.c | 19 +++++++++++++++---- > net/sctp/offload.c | 2 ++ > 4 files changed, 33 insertions(+), 8 deletions(-) > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > index 8311c38267b5..f9b93708c22e 100644 > --- a/net/ipv4/tcp_offload.c > +++ b/net/ipv4/tcp_offload.c > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, > /* Packet is from an untrusted source, reset gso_segs. */ > > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > segs = NULL; > goto out; > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > index 75aa4de5b731..bd29cf19bb6b 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > goto out; > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > - return __udp_gso_segment(skb, features, false); > - > mss = skb_shinfo(skb)->gso_size; Why move the block below this line? > + > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > + /* Packet is from an untrusted source, reset actual gso_segs */ > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > + mss); > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > + > + segs = NULL; > + goto out; > + } else { > + return __udp_gso_segment(skb, features, false); > + } > + } > + The validation should take place inside __udp_gso_segment. Revert the previous patch to always enter that function for USO packets: if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) return __udp_gso_segment(skb, features, false); And in that function decide to return NULL after validation. > if (unlikely(skb->len <= mss)) > goto out; > > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > index ad3b8726873e..6857d9f7bd06 100644 > --- a/net/ipv6/udp_offload.c > +++ b/net/ipv6/udp_offload.c > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > goto out; > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > - return __udp_gso_segment(skb, features, true); > - > mss = skb_shinfo(skb)->gso_size; > + > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > + /* Packet is from an untrusted source, reset actual gso_segs */ > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > + mss); > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > + > + segs = NULL; > + goto out; > + } else { > + return __udp_gso_segment(skb, features, true); > + } > + } > + > if (unlikely(skb->len <= mss)) > goto out; > > diff --git a/net/sctp/offload.c b/net/sctp/offload.c > index 502095173d88..3d2b44db0d42 100644 > --- a/net/sctp/offload.c > +++ b/net/sctp/offload.c > @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb, > skb_walk_frags(skb, frag_iter) > pinfo->gso_segs++; > > + pinfo->gso_type &= ~SKB_GSO_DODGY; > + > segs = NULL; > goto out; > } > -- > 2.30.2 >
On Thu, Jul 13, 2023 at 9:55 AM Yan Zhai <yan@cloudflare.com> wrote: > > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source. > The canonical way is to recompute the gso_segs to avoid device driver > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the > egress of later devices, e.g. packets can egress to a vlan device backed > by a real NIC. > > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 > packets.") checks DODGY bit for UDP, but for packets that can be fed > directly to the device after gso_segs reset, it actually falls through > to fragmentation [1]. > > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417 > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY > bit after recomputing gso_segs. If we try to fix two issues, we'd better use separate patches. > > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit > at other places. > > Fixes: 90017accff61 ("sctp: Add GSO support") > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > --- > [1]: > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > --- > net/ipv4/tcp_offload.c | 1 + > net/ipv4/udp_offload.c | 19 +++++++++++++++---- > net/ipv6/udp_offload.c | 19 +++++++++++++++---- > net/sctp/offload.c | 2 ++ > 4 files changed, 33 insertions(+), 8 deletions(-) > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > index 8311c38267b5..f9b93708c22e 100644 > --- a/net/ipv4/tcp_offload.c > +++ b/net/ipv4/tcp_offload.c > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, > /* Packet is from an untrusted source, reset gso_segs. */ > > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > segs = NULL; > goto out; > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > index 75aa4de5b731..bd29cf19bb6b 100644 > --- a/net/ipv4/udp_offload.c > +++ b/net/ipv4/udp_offload.c > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > goto out; > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > - return __udp_gso_segment(skb, features, false); > - > mss = skb_shinfo(skb)->gso_size; > + > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > + /* Packet is from an untrusted source, reset actual gso_segs */ > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > + mss); > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > + > + segs = NULL; > + goto out; > + } else { > + return __udp_gso_segment(skb, features, false); I think it's better and cleaner to move those changes in __udp_gso_segment() as Willem suggests. > + } > + } > + > if (unlikely(skb->len <= mss)) > goto out; > > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > index ad3b8726873e..6857d9f7bd06 100644 > --- a/net/ipv6/udp_offload.c > +++ b/net/ipv6/udp_offload.c > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > goto out; > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > - return __udp_gso_segment(skb, features, true); > - > mss = skb_shinfo(skb)->gso_size; > + > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > + /* Packet is from an untrusted source, reset actual gso_segs */ > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > + mss); > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; Any reason you want to remove the DODGY here? Is this an optimization? We will lose the chance to recognize/validate it elsewhere. Thanks > + > + segs = NULL; > + goto out; > + } else { > + return __udp_gso_segment(skb, features, true); > + } > + } > + > if (unlikely(skb->len <= mss)) > goto out; > > diff --git a/net/sctp/offload.c b/net/sctp/offload.c > index 502095173d88..3d2b44db0d42 100644 > --- a/net/sctp/offload.c > +++ b/net/sctp/offload.c > @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb, > skb_walk_frags(skb, frag_iter) > pinfo->gso_segs++; > > + pinfo->gso_type &= ~SKB_GSO_DODGY; > + > segs = NULL; > goto out; > } > -- > 2.30.2 >
On Thu, Jul 13, 2023 at 10:02 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Wed, Jul 12, 2023 at 9:55 PM Yan Zhai <yan@cloudflare.com> wrote: > > > > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source. > > The canonical way is to recompute the gso_segs to avoid device driver > > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the > > egress of later devices, e.g. packets can egress to a vlan device backed > > by a real NIC. > > > > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 > > packets.") checks DODGY bit for UDP, but for packets that can be fed > > directly to the device after gso_segs reset, it actually falls through > > to fragmentation [1]. > > > > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417 > > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY > > bit after recomputing gso_segs. > > > > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit > > at other places. > > These two things should not be conflated. > > Only the USO fix is strictly needed to fix the reported issue. > > > Fixes: 90017accff61 ("sctp: Add GSO support") > > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") > > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") > > Link: https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > > > --- > > [1]: > > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > > > --- > > net/ipv4/tcp_offload.c | 1 + > > net/ipv4/udp_offload.c | 19 +++++++++++++++---- > > net/ipv6/udp_offload.c | 19 +++++++++++++++---- > > net/sctp/offload.c | 2 ++ > > 4 files changed, 33 insertions(+), 8 deletions(-) > > > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > > index 8311c38267b5..f9b93708c22e 100644 > > --- a/net/ipv4/tcp_offload.c > > +++ b/net/ipv4/tcp_offload.c > > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, > > /* Packet is from an untrusted source, reset gso_segs. */ > > > > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > > > segs = NULL; > > goto out; > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > index 75aa4de5b731..bd29cf19bb6b 100644 > > --- a/net/ipv4/udp_offload.c > > +++ b/net/ipv4/udp_offload.c > > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > goto out; > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > - return __udp_gso_segment(skb, features, false); > > - > > mss = skb_shinfo(skb)->gso_size; > > Why move the block below this line? > > > + > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > + mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > + > > + segs = NULL; > > + goto out; > > + } else { > > + return __udp_gso_segment(skb, features, false); > > + } > > + } > > + > > The validation should take place inside __udp_gso_segment. > > Revert the previous patch to always enter that function for USO packets: > > if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) > return __udp_gso_segment(skb, features, false); > > And in that function decide to return NULL after validation. +1 Thanks > > > > if (unlikely(skb->len <= mss)) > > goto out; > > > > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > > index ad3b8726873e..6857d9f7bd06 100644 > > --- a/net/ipv6/udp_offload.c > > +++ b/net/ipv6/udp_offload.c > > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > goto out; > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > - return __udp_gso_segment(skb, features, true); > > - > > mss = skb_shinfo(skb)->gso_size; > > + > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > + mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > + > > + segs = NULL; > > + goto out; > > + } else { > > + return __udp_gso_segment(skb, features, true); > > + } > > + } > > + > > if (unlikely(skb->len <= mss)) > > goto out; > > > > diff --git a/net/sctp/offload.c b/net/sctp/offload.c > > index 502095173d88..3d2b44db0d42 100644 > > --- a/net/sctp/offload.c > > +++ b/net/sctp/offload.c > > @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb, > > skb_walk_frags(skb, frag_iter) > > pinfo->gso_segs++; > > > > + pinfo->gso_type &= ~SKB_GSO_DODGY; > > + > > segs = NULL; > > goto out; > > } > > -- > > 2.30.2 > > >
On Wed, Jul 12, 2023 at 9:02 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Wed, Jul 12, 2023 at 9:55 PM Yan Zhai <yan@cloudflare.com> wrote: > > > > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source. > > The canonical way is to recompute the gso_segs to avoid device driver > > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the > > egress of later devices, e.g. packets can egress to a vlan device backed > > by a real NIC. > > > > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 > > packets.") checks DODGY bit for UDP, but for packets that can be fed > > directly to the device after gso_segs reset, it actually falls through > > to fragmentation [1]. > > > > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417 > > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY > > bit after recomputing gso_segs. > > > > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit > > at other places. > > These two things should not be conflated. > > Only the USO fix is strictly needed to fix the reported issue. > It's my OCD of wanting to avoid a cover letter for two patches... Let's address just this UDP issue then this time. The removal of DODGY is in fact more suitable as RFC for small improvements. > > Fixes: 90017accff61 ("sctp: Add GSO support") > > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") > > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") > > Link: https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > > > --- > > [1]: > > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > > > --- > > net/ipv4/tcp_offload.c | 1 + > > net/ipv4/udp_offload.c | 19 +++++++++++++++---- > > net/ipv6/udp_offload.c | 19 +++++++++++++++---- > > net/sctp/offload.c | 2 ++ > > 4 files changed, 33 insertions(+), 8 deletions(-) > > > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > > index 8311c38267b5..f9b93708c22e 100644 > > --- a/net/ipv4/tcp_offload.c > > +++ b/net/ipv4/tcp_offload.c > > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, > > /* Packet is from an untrusted source, reset gso_segs. */ > > > > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > > > segs = NULL; > > goto out; > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > index 75aa4de5b731..bd29cf19bb6b 100644 > > --- a/net/ipv4/udp_offload.c > > +++ b/net/ipv4/udp_offload.c > > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > goto out; > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > - return __udp_gso_segment(skb, features, false); > > - > > mss = skb_shinfo(skb)->gso_size; > > Why move the block below this line? > if we move the dodgy handling into __udp_gso_segment then it does not need to move below this line. > > + > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > + mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > + > > + segs = NULL; > > + goto out; > > + } else { > > + return __udp_gso_segment(skb, features, false); > > + } > > + } > > + > > The validation should take place inside __udp_gso_segment. > > Revert the previous patch to always enter that function for USO packets: > > if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) > return __udp_gso_segment(skb, features, false); > > And in that function decide to return NULL after validation. > Good call, that's indeed better. Thanks > > > if (unlikely(skb->len <= mss)) > > goto out; > > > > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > > index ad3b8726873e..6857d9f7bd06 100644 > > --- a/net/ipv6/udp_offload.c > > +++ b/net/ipv6/udp_offload.c > > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > goto out; > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > - return __udp_gso_segment(skb, features, true); > > - > > mss = skb_shinfo(skb)->gso_size; > > + > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > + mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > + > > + segs = NULL; > > + goto out; > > + } else { > > + return __udp_gso_segment(skb, features, true); > > + } > > + } > > + > > if (unlikely(skb->len <= mss)) > > goto out; > > > > diff --git a/net/sctp/offload.c b/net/sctp/offload.c > > index 502095173d88..3d2b44db0d42 100644 > > --- a/net/sctp/offload.c > > +++ b/net/sctp/offload.c > > @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb, > > skb_walk_frags(skb, frag_iter) > > pinfo->gso_segs++; > > > > + pinfo->gso_type &= ~SKB_GSO_DODGY; > > + > > segs = NULL; > > goto out; > > } > > -- > > 2.30.2 > >
On Wed, Jul 12, 2023 at 9:11 PM Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Jul 13, 2023 at 9:55 AM Yan Zhai <yan@cloudflare.com> wrote: > > > > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source. > > The canonical way is to recompute the gso_segs to avoid device driver > > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the > > egress of later devices, e.g. packets can egress to a vlan device backed > > by a real NIC. > > > > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 > > packets.") checks DODGY bit for UDP, but for packets that can be fed > > directly to the device after gso_segs reset, it actually falls through > > to fragmentation [1]. > > > > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417 > > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY > > bit after recomputing gso_segs. > > If we try to fix two issues, we'd better use separate patches. > > > > > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit > > at other places. > > > > Fixes: 90017accff61 ("sctp: Add GSO support") > > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") > > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") > > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > > > --- > > [1]: > > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > > > --- > > net/ipv4/tcp_offload.c | 1 + > > net/ipv4/udp_offload.c | 19 +++++++++++++++---- > > net/ipv6/udp_offload.c | 19 +++++++++++++++---- > > net/sctp/offload.c | 2 ++ > > 4 files changed, 33 insertions(+), 8 deletions(-) > > > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > > index 8311c38267b5..f9b93708c22e 100644 > > --- a/net/ipv4/tcp_offload.c > > +++ b/net/ipv4/tcp_offload.c > > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, > > /* Packet is from an untrusted source, reset gso_segs. */ > > > > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > > > segs = NULL; > > goto out; > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > index 75aa4de5b731..bd29cf19bb6b 100644 > > --- a/net/ipv4/udp_offload.c > > +++ b/net/ipv4/udp_offload.c > > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > goto out; > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > - return __udp_gso_segment(skb, features, false); > > - > > mss = skb_shinfo(skb)->gso_size; > > + > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > + mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > + > > + segs = NULL; > > + goto out; > > + } else { > > + return __udp_gso_segment(skb, features, false); > > I think it's better and cleaner to move those changes in > __udp_gso_segment() as Willem suggests. > > > + } > > + } > > + > > if (unlikely(skb->len <= mss)) > > goto out; > > > > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > > index ad3b8726873e..6857d9f7bd06 100644 > > --- a/net/ipv6/udp_offload.c > > +++ b/net/ipv6/udp_offload.c > > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > goto out; > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > - return __udp_gso_segment(skb, features, true); > > - > > mss = skb_shinfo(skb)->gso_size; > > + > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > + mss); > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > Any reason you want to remove the DODGY here? Is this an optimization? > We will lose the chance to recognize/validate it elsewhere. > It is intended as a small optimization. And this is in fact the piece I am not fully confident about: after validating the gso_segs at a trusted location (i.e. assuming the kernel is the trusted computing base), do we need to validate it somewhere else? For example, in our scenario, we have a tun/tap device in a net namespace, so the packet going out will enter from the tap, get forwarded through an veth, and then a vlan backed by a real ethernet interface. If the bit is carried over, then at each egress of these devices, we need to enter the GSO code, which feels pretty redundant as long as the packet does not leave kernel space. WDYT? thanks > Thanks > > > + > > + segs = NULL; > > + goto out; > > + } else { > > + return __udp_gso_segment(skb, features, true); > > + } > > + } > > + > > if (unlikely(skb->len <= mss)) > > goto out; > > > > diff --git a/net/sctp/offload.c b/net/sctp/offload.c > > index 502095173d88..3d2b44db0d42 100644 > > --- a/net/sctp/offload.c > > +++ b/net/sctp/offload.c > > @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb, > > skb_walk_frags(skb, frag_iter) > > pinfo->gso_segs++; > > > > + pinfo->gso_type &= ~SKB_GSO_DODGY; > > + > > segs = NULL; > > goto out; > > } > > -- > > 2.30.2 > > >
On Wed, 2023-07-12 at 21:58 -0500, Yan Zhai wrote: > On Wed, Jul 12, 2023 at 9:11 PM Jason Wang <jasowang@redhat.com> wrote: > > > > On Thu, Jul 13, 2023 at 9:55 AM Yan Zhai <yan@cloudflare.com> wrote: > > > > > > SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source. > > > The canonical way is to recompute the gso_segs to avoid device driver > > > issues. Afterwards, the DODGY bit can be removed to avoid re-check at the > > > egress of later devices, e.g. packets can egress to a vlan device backed > > > by a real NIC. > > > > > > Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 > > > packets.") checks DODGY bit for UDP, but for packets that can be fed > > > directly to the device after gso_segs reset, it actually falls through > > > to fragmentation [1]. > > > > > > Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417 > > > ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY > > > bit after recomputing gso_segs. > > > > If we try to fix two issues, we'd better use separate patches. > > > > > > > > This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit > > > at other places. > > > > > > Fixes: 90017accff61 ("sctp: Add GSO support") > > > Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") > > > Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") > > > Signed-off-by: Yan Zhai <yan@cloudflare.com> > > > > > > --- > > > [1]: > > > https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ > > > > > > --- > > > net/ipv4/tcp_offload.c | 1 + > > > net/ipv4/udp_offload.c | 19 +++++++++++++++---- > > > net/ipv6/udp_offload.c | 19 +++++++++++++++---- > > > net/sctp/offload.c | 2 ++ > > > 4 files changed, 33 insertions(+), 8 deletions(-) > > > > > > diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c > > > index 8311c38267b5..f9b93708c22e 100644 > > > --- a/net/ipv4/tcp_offload.c > > > +++ b/net/ipv4/tcp_offload.c > > > @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, > > > /* Packet is from an untrusted source, reset gso_segs. */ > > > > > > skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); > > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > > > > > segs = NULL; > > > goto out; > > > diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c > > > index 75aa4de5b731..bd29cf19bb6b 100644 > > > --- a/net/ipv4/udp_offload.c > > > +++ b/net/ipv4/udp_offload.c > > > @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, > > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > > goto out; > > > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > > - return __udp_gso_segment(skb, features, false); > > > - > > > mss = skb_shinfo(skb)->gso_size; > > > + > > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > > + mss); > > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > > + > > > + segs = NULL; > > > + goto out; > > > + } else { > > > + return __udp_gso_segment(skb, features, false); > > > > I think it's better and cleaner to move those changes in > > __udp_gso_segment() as Willem suggests. > > > > > + } > > > + } > > > + > > > if (unlikely(skb->len <= mss)) > > > goto out; > > > > > > diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c > > > index ad3b8726873e..6857d9f7bd06 100644 > > > --- a/net/ipv6/udp_offload.c > > > +++ b/net/ipv6/udp_offload.c > > > @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, > > > if (!pskb_may_pull(skb, sizeof(struct udphdr))) > > > goto out; > > > > > > - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && > > > - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) > > > - return __udp_gso_segment(skb, features, true); > > > - > > > mss = skb_shinfo(skb)->gso_size; > > > + > > > + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { > > > + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { > > > + /* Packet is from an untrusted source, reset actual gso_segs */ > > > + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), > > > + mss); > > > + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; > > > > Any reason you want to remove the DODGY here? Is this an optimization? > > We will lose the chance to recognize/validate it elsewhere. > > > It is intended as a small optimization. And this is in fact the piece > I am not fully confident about: after validating the gso_segs at a > trusted location (i.e. assuming the kernel is the trusted computing > base), do we need to validate it somewhere else? For example, in our > scenario, we have a tun/tap device in a net namespace, so the packet > going out will enter from the tap, get forwarded through an veth, and > then a vlan backed by a real ethernet interface. If the bit is carried > over, then at each egress of these devices, we need to enter the GSO > code, which feels pretty redundant as long as the packet does not > leave kernel space. WDYT? As an optimization, I think it should land on a different (net-next) patch. Additionally I think it should be possible to get a greater gain adding the ROBUST feature to virtual devices (but I'm not sure if syzkaller will be able to use that in nasty ways). Cheers, Paolo
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c index 8311c38267b5..f9b93708c22e 100644 --- a/net/ipv4/tcp_offload.c +++ b/net/ipv4/tcp_offload.c @@ -87,6 +87,7 @@ struct sk_buff *tcp_gso_segment(struct sk_buff *skb, /* Packet is from an untrusted source, reset gso_segs. */ skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len, mss); + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; segs = NULL; goto out; diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c index 75aa4de5b731..bd29cf19bb6b 100644 --- a/net/ipv4/udp_offload.c +++ b/net/ipv4/udp_offload.c @@ -388,11 +388,22 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb, if (!pskb_may_pull(skb, sizeof(struct udphdr))) goto out; - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) - return __udp_gso_segment(skb, features, false); - mss = skb_shinfo(skb)->gso_size; + + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { + /* Packet is from an untrusted source, reset actual gso_segs */ + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), + mss); + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; + + segs = NULL; + goto out; + } else { + return __udp_gso_segment(skb, features, false); + } + } + if (unlikely(skb->len <= mss)) goto out; diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c index ad3b8726873e..6857d9f7bd06 100644 --- a/net/ipv6/udp_offload.c +++ b/net/ipv6/udp_offload.c @@ -43,11 +43,22 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, if (!pskb_may_pull(skb, sizeof(struct udphdr))) goto out; - if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4 && - !skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) - return __udp_gso_segment(skb, features, true); - mss = skb_shinfo(skb)->gso_size; + + if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4) { + if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) { + /* Packet is from an untrusted source, reset actual gso_segs */ + skb_shinfo(skb)->gso_segs = DIV_ROUND_UP(skb->len - sizeof(*uh), + mss); + skb_shinfo(skb)->gso_type &= ~SKB_GSO_DODGY; + + segs = NULL; + goto out; + } else { + return __udp_gso_segment(skb, features, true); + } + } + if (unlikely(skb->len <= mss)) goto out; diff --git a/net/sctp/offload.c b/net/sctp/offload.c index 502095173d88..3d2b44db0d42 100644 --- a/net/sctp/offload.c +++ b/net/sctp/offload.c @@ -65,6 +65,8 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb, skb_walk_frags(skb, frag_iter) pinfo->gso_segs++; + pinfo->gso_type &= ~SKB_GSO_DODGY; + segs = NULL; goto out; }
SKB_GSO_DODGY bit indicates a GSO packet comes from an untrusted source. The canonical way is to recompute the gso_segs to avoid device driver issues. Afterwards, the DODGY bit can be removed to avoid re-check at the egress of later devices, e.g. packets can egress to a vlan device backed by a real NIC. Commit 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") checks DODGY bit for UDP, but for packets that can be fed directly to the device after gso_segs reset, it actually falls through to fragmentation [1]. Commit 90017accff61 ("sctp: Add GSO support") and commit 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") both didn't remove the DODGY bit after recomputing gso_segs. This change fixes the GSO_UDP_L4 handling case, and remove the DODGY bit at other places. Fixes: 90017accff61 ("sctp: Add GSO support") Fixes: 3820c3f3e417 ("[TCP]: Reset gso_segs if packet is dodgy") Fixes: 1fd54773c267 ("udp: allow header check for dodgy GSO_UDP_L4 packets.") Signed-off-by: Yan Zhai <yan@cloudflare.com> --- [1]: https://lore.kernel.org/all/CAJPywTKDdjtwkLVUW6LRA2FU912qcDmQOQGt2WaDo28KzYDg+A@mail.gmail.com/ --- net/ipv4/tcp_offload.c | 1 + net/ipv4/udp_offload.c | 19 +++++++++++++++---- net/ipv6/udp_offload.c | 19 +++++++++++++++---- net/sctp/offload.c | 2 ++ 4 files changed, 33 insertions(+), 8 deletions(-)