| Message ID | 20231202161441.221135-1-syoshida@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 80d875cfc9d3711a029f234ef7d680db79e8fa4b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] ipv4: ip_gre: Avoid skb_pull() failure in ipgre_xmit() | expand |
Hi Shigeru, >diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index >22a26d1d29a0..5169c3c72cff 100644 >--- a/net/ipv4/ip_gre.c >+++ b/net/ipv4/ip_gre.c >@@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > } > > if (dev->header_ops) { >+ int pull_len = tunnel->hlen + sizeof(struct iphdr); >+ > if (skb_cow_head(skb, 0)) > goto free_skb; > > tnl_params = (const struct iphdr *)skb->data; > >- /* Pull skb since ip_tunnel_xmit() needs skb->data pointing >- * to gre header. >- */ >- skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); >+ if (!pskb_network_may_pull(skb, pull_len)) [Suman] Since this is transmit path, should we add unlikely() here? >+ goto free_skb; >+ >+ /* ip_tunnel_xmit() needs skb->data pointing to gre header. */ >+ skb_pull(skb, pull_len); > skb_reset_mac_header(skb); > > if (skb->ip_summed == CHECKSUM_PARTIAL && >-- >2.41.0 >
On Sun, Dec 3, 2023 at 7:58 AM Suman Ghosh <sumang@marvell.com> wrote: > > Hi Shigeru, > > >diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index > >22a26d1d29a0..5169c3c72cff 100644 > >--- a/net/ipv4/ip_gre.c > >+++ b/net/ipv4/ip_gre.c > >@@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, > > } > > > > if (dev->header_ops) { > >+ int pull_len = tunnel->hlen + sizeof(struct iphdr); > >+ > > if (skb_cow_head(skb, 0)) > > goto free_skb; > > > > tnl_params = (const struct iphdr *)skb->data; > > > >- /* Pull skb since ip_tunnel_xmit() needs skb->data pointing > >- * to gre header. > >- */ > >- skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); > >+ if (!pskb_network_may_pull(skb, pull_len)) > [Suman] Since this is transmit path, should we add unlikely() here? Adding unlikely() is not needed, it is already done generically from the inline helpers. Reviewed-by: Eric Dumazet <edumazet@google.com>
Hi Suman, On Sun, 3 Dec 2023 06:58:19 +0000, Suman Ghosh wrote: > Hi Shigeru, > >>diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index >>22a26d1d29a0..5169c3c72cff 100644 >>--- a/net/ipv4/ip_gre.c >>+++ b/net/ipv4/ip_gre.c >>@@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, >> } >> >> if (dev->header_ops) { >>+ int pull_len = tunnel->hlen + sizeof(struct iphdr); >>+ >> if (skb_cow_head(skb, 0)) >> goto free_skb; >> >> tnl_params = (const struct iphdr *)skb->data; >> >>- /* Pull skb since ip_tunnel_xmit() needs skb->data pointing >>- * to gre header. >>- */ >>- skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); >>+ if (!pskb_network_may_pull(skb, pull_len)) > [Suman] Since this is transmit path, should we add unlikely() here? Thanks for your comment. I traced this function and found that pskb_may_pull_reason() seems to have appropriate likely() and unlikely() as Eric says. I'm new to Linux networking. Could you kindly explain the background of your suggestion? I understand that a transmit path must be as fast as possible, so we should use unlikely() for rare cases such like this error path. Am I correct? Thanks, Shigeru >>+ goto free_skb; >>+ >>+ /* ip_tunnel_xmit() needs skb->data pointing to gre header. */ >>+ skb_pull(skb, pull_len); >> skb_reset_mac_header(skb); >> >> if (skb->ip_summed == CHECKSUM_PARTIAL && >>-- >>2.41.0 >> >
>>> } >>> >>> if (dev->header_ops) { >>>+ int pull_len = tunnel->hlen + sizeof(struct iphdr); >>>+ >>> if (skb_cow_head(skb, 0)) >>> goto free_skb; >>> >>> tnl_params = (const struct iphdr *)skb->data; >>> >>>- /* Pull skb since ip_tunnel_xmit() needs skb->data pointing >>>- * to gre header. >>>- */ >>>- skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); >>>+ if (!pskb_network_may_pull(skb, pull_len)) >> [Suman] Since this is transmit path, should we add unlikely() here? > >Thanks for your comment. > >I traced this function and found that pskb_may_pull_reason() seems to >have appropriate likely() and unlikely() as Eric says. > >I'm new to Linux networking. Could you kindly explain the background of >your suggestion? > >I understand that a transmit path must be as fast as possible, so we >should use unlikely() for rare cases such like this error path. Am I >correct? > >Thanks, >Shigeru [Suman] Yes. Likely()/unlikely() helps the compiler for branch prediction and we use it mostly on the data path. But I cross checked that this is static inline and the function pskb_may_pull() already have likely()/unlikely() in place. So, you can ignore my comment here. > >>>+ goto free_skb; >>>+ >>>+ /* ip_tunnel_xmit() needs skb->data pointing to gre header. */ >>>+ skb_pull(skb, pull_len); >>> skb_reset_mac_header(skb); >>> >>> if (skb->ip_summed == CHECKSUM_PARTIAL && >>>-- >>>2.41.0 >>> >>
>In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() >returns true. For example, applications can use PF_PACKET to create a >malformed packet with no IP header. This type of packet causes a problem >such as uninit-value access. > >This patch ensures that skb_pull() can pull the required size by >checking the skb with pskb_network_may_pull() before skb_pull(). > >Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") >Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> >--- Reviewed-by: Suman Ghosh <sumang@marvell.com> >v1 -> v2: >- Change the title >- Update the code with Eric's suggestion >
On Sun, 3 Dec 2023 15:17:09 +0000, Suman Ghosh wrote: >>>> } >>>> >>>> if (dev->header_ops) { >>>>+ int pull_len = tunnel->hlen + sizeof(struct iphdr); >>>>+ >>>> if (skb_cow_head(skb, 0)) >>>> goto free_skb; >>>> >>>> tnl_params = (const struct iphdr *)skb->data; >>>> >>>>- /* Pull skb since ip_tunnel_xmit() needs skb->data pointing >>>>- * to gre header. >>>>- */ >>>>- skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); >>>>+ if (!pskb_network_may_pull(skb, pull_len)) >>> [Suman] Since this is transmit path, should we add unlikely() here? >> >>Thanks for your comment. >> >>I traced this function and found that pskb_may_pull_reason() seems to >>have appropriate likely() and unlikely() as Eric says. >> >>I'm new to Linux networking. Could you kindly explain the background of >>your suggestion? >> >>I understand that a transmit path must be as fast as possible, so we >>should use unlikely() for rare cases such like this error path. Am I >>correct? >> >>Thanks, >>Shigeru > [Suman] Yes. Likely()/unlikely() helps the compiler for branch prediction and we use it mostly on the data path. > But I cross checked that this is static inline and the function pskb_may_pull() already have likely()/unlikely() in place. > So, you can ignore my comment here. Thank you for your explanation. It is very informative. And thanks for the review as well. Shigeru >> >>>>+ goto free_skb; >>>>+ >>>>+ /* ip_tunnel_xmit() needs skb->data pointing to gre header. */ >>>>+ skb_pull(skb, pull_len); >>>> skb_reset_mac_header(skb); >>>> >>>> if (skb->ip_summed == CHECKSUM_PARTIAL && >>>>-- >>>>2.41.0 >>>> >>> >
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Sun, 3 Dec 2023 01:14:41 +0900 you wrote: > In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns > true. For example, applications can use PF_PACKET to create a malformed > packet with no IP header. This type of packet causes a problem such as > uninit-value access. > > This patch ensures that skb_pull() can pull the required size by checking > the skb with pskb_network_may_pull() before skb_pull(). > > [...] Here is the summary with links: - [net,v2] ipv4: ip_gre: Avoid skb_pull() failure in ipgre_xmit() https://git.kernel.org/netdev/net/c/80d875cfc9d3 You are awesome, thank you!
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c index 22a26d1d29a0..5169c3c72cff 100644 --- a/net/ipv4/ip_gre.c +++ b/net/ipv4/ip_gre.c @@ -635,15 +635,18 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb, } if (dev->header_ops) { + int pull_len = tunnel->hlen + sizeof(struct iphdr); + if (skb_cow_head(skb, 0)) goto free_skb; tnl_params = (const struct iphdr *)skb->data; - /* Pull skb since ip_tunnel_xmit() needs skb->data pointing - * to gre header. - */ - skb_pull(skb, tunnel->hlen + sizeof(struct iphdr)); + if (!pskb_network_may_pull(skb, pull_len)) + goto free_skb; + + /* ip_tunnel_xmit() needs skb->data pointing to gre header. */ + skb_pull(skb, pull_len); skb_reset_mac_header(skb); if (skb->ip_summed == CHECKSUM_PARTIAL &&
In ipgre_xmit(), skb_pull() may fail even if pskb_inet_may_pull() returns true. For example, applications can use PF_PACKET to create a malformed packet with no IP header. This type of packet causes a problem such as uninit-value access. This patch ensures that skb_pull() can pull the required size by checking the skb with pskb_network_may_pull() before skb_pull(). Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.") Signed-off-by: Shigeru Yoshida <syoshida@redhat.com> --- v1 -> v2: - Change the title - Update the code with Eric's suggestion https://lore.kernel.org/all/20231126151652.372783-1-syoshida@redhat.com/ --- net/ipv4/ip_gre.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)