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 <[email protected]>
---
v1 -> v2:
- Change the title
- Update the code with Eric's suggestion
https://lore.kernel.org/all/[email protected]/
---
net/ipv4/ip_gre.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
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 &&
--
2.41.0
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 <[email protected]> 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 <[email protected]>
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 <[email protected]>
>---
Reviewed-by: Suman Ghosh <[email protected]>
>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 <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html