[PATCH net] ip_gre: validate csum_start only on pull

[PATCH net] ip_gre: validate csum_start only on pull

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

The GRE tunnel device can pull existing outer headers in ipge_xmit.
This is a rare path, apparently unique to this device. The below
commit ensured that pulling does not move skb->data beyond csum_start.

But it has a false positive if ip_summed is not CHECKSUM_PARTIAL and
thus csum_start is irrelevant.

Refine to exclude this. At the same time simplify and strengthen the
test.

Simplify, by moving the check next to the offending pull, making it
more self documenting and removing an unnecessary branch from other
code paths.

Strengthen, by also ensuring that the transport header is correct and
therefore the inner headers will be after skb_reset_inner_headers.
The transport header is set to csum_start in skb_partial_csum_set.

Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
Fixes: 1d011c4803c7 ("ip_gre: add validation for csum_start")
Reported-by: Ido Schimmel <idosch@idosch.org>
Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 net/ipv4/ip_gre.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 177d26d8fb9c..0fe6c936dc54 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -473,8 +473,6 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev,
 
 static int gre_handle_offloads(struct sk_buff *skb, bool csum)
 {
-	if (csum && skb_checksum_start(skb) < skb->data)
-		return -EINVAL;
 	return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
 }
 
@@ -632,15 +630,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
 	}
 
 	if (dev->header_ops) {
+		const int pull_len = tunnel->hlen + sizeof(struct iphdr);
+
 		if (skb_cow_head(skb, 0))
 			goto free_skb;
 
 		tnl_params = (const struct iphdr *)skb->data;
 
+		if (pull_len > skb_transport_offset(skb))
+			goto free_skb;
+
 		/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
 		 * to gre header.
 		 */
-		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
+		skb_pull(skb, pull_len);
 		skb_reset_mac_header(skb);
 	} else {
 		if (skb_cow_head(skb, dev->needed_headroom))
-- 
2.33.0.153.gba50c8fa24-goog

Re: [PATCH net] ip_gre: validate csum_start only on pull

[Alexander Duyck]

On Sun, Sep 5, 2021 at 8:21 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> The GRE tunnel device can pull existing outer headers in ipge_xmit.
> This is a rare path, apparently unique to this device. The below
> commit ensured that pulling does not move skb->data beyond csum_start.
>
> But it has a false positive if ip_summed is not CHECKSUM_PARTIAL and
> thus csum_start is irrelevant.
>
> Refine to exclude this. At the same time simplify and strengthen the
> test.
>
> Simplify, by moving the check next to the offending pull, making it
> more self documenting and removing an unnecessary branch from other
> code paths.
>
> Strengthen, by also ensuring that the transport header is correct and
> therefore the inner headers will be after skb_reset_inner_headers.
> The transport header is set to csum_start in skb_partial_csum_set.
>
> Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
> Fixes: 1d011c4803c7 ("ip_gre: add validation for csum_start")
> Reported-by: Ido Schimmel <idosch@idosch.org>
> Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
> ---
>  net/ipv4/ip_gre.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 177d26d8fb9c..0fe6c936dc54 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -473,8 +473,6 @@ static void __gre_xmit(struct sk_buff *skb, struct net_device *dev,
>
>  static int gre_handle_offloads(struct sk_buff *skb, bool csum)
>  {
> -       if (csum && skb_checksum_start(skb) < skb->data)
> -               return -EINVAL;
>         return iptunnel_handle_offloads(skb, csum ? SKB_GSO_GRE_CSUM : SKB_GSO_GRE);
>  }
>
> @@ -632,15 +630,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>         }
>
>         if (dev->header_ops) {
> +               const int pull_len = tunnel->hlen + sizeof(struct iphdr);
> +
>                 if (skb_cow_head(skb, 0))
>                         goto free_skb;
>
>                 tnl_params = (const struct iphdr *)skb->data;
>
> +               if (pull_len > skb_transport_offset(skb))
> +                       goto free_skb;
> +
>                 /* Pull skb since ip_tunnel_xmit() needs skb->data pointing
>                  * to gre header.
>                  */
> -               skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
> +               skb_pull(skb, pull_len);
>                 skb_reset_mac_header(skb);
>         } else {
>                 if (skb_cow_head(skb, dev->needed_headroom))
> --
> 2.33.0.153.gba50c8fa24-goog
>

Looks good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Re: [PATCH net] ip_gre: validate csum_start only on pull

[Ido Schimmel]

On Sun, Sep 05, 2021 at 08:47:16AM -0700, Alexander Duyck wrote:
> Looks good to me.
> 
> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Thanks Willem and Alex! Applied the patch to my tree. Will let you know
tomorrow morning after regression is complete (though I'm sure it's
fine).

Re: [PATCH net] ip_gre: validate csum_start only on pull

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Sun,  5 Sep 2021 11:21:09 -0400 you wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> The GRE tunnel device can pull existing outer headers in ipge_xmit.
> This is a rare path, apparently unique to this device. The below
> commit ensured that pulling does not move skb->data beyond csum_start.
> 
> But it has a false positive if ip_summed is not CHECKSUM_PARTIAL and
> thus csum_start is irrelevant.
> 
> [...]

Here is the summary with links:
  - [net] ip_gre: validate csum_start only on pull
    https://git.kernel.org/netdev/net/c/8a0ed250f911

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] ip_gre: validate csum_start only on pull

[Ido Schimmel]

On Sun, Sep 05, 2021 at 11:21:09AM -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
> 
> The GRE tunnel device can pull existing outer headers in ipge_xmit.
> This is a rare path, apparently unique to this device. The below
> commit ensured that pulling does not move skb->data beyond csum_start.
> 
> But it has a false positive if ip_summed is not CHECKSUM_PARTIAL and
> thus csum_start is irrelevant.
> 
> Refine to exclude this. At the same time simplify and strengthen the
> test.
> 
> Simplify, by moving the check next to the offending pull, making it
> more self documenting and removing an unnecessary branch from other
> code paths.
> 
> Strengthen, by also ensuring that the transport header is correct and
> therefore the inner headers will be after skb_reset_inner_headers.
> The transport header is set to csum_start in skb_partial_csum_set.
> 
> Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
> Fixes: 1d011c4803c7 ("ip_gre: add validation for csum_start")
> Reported-by: Ido Schimmel <idosch@idosch.org>
> Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

FTR:

Tested-by: Ido Schimmel <idosch@nvidia.com>

Thanks