Skip to content

Commit

Permalink
ip_gre: test csum_start instead of transport header
Browse files Browse the repository at this point in the history
GRE with TUNNEL_CSUM will apply local checksum offload on
CHECKSUM_PARTIAL packets.

ipgre_xmit must validate csum_start after an optional skb_pull,
else lco_csum may trigger an overflow. The original check was

	if (csum && skb_checksum_start(skb) < skb->data)
		return -EINVAL;

This had false positives when skb_checksum_start is undefined:
when ip_summed is not CHECKSUM_PARTIAL. A discussed refinement
was straightforward

	if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
	    skb_checksum_start(skb) < skb->data)
		return -EINVAL;

But was eventually revised more thoroughly:
- restrict the check to the only branch where needed, in an
  uncommon GRE path that uses header_ops and calls skb_pull.
- test skb_transport_header, which is set along with csum_start
  in skb_partial_csum_set in the normal header_ops datapath.

Turns out skbs can arrive in this branch without the transport
header set, e.g., through BPF redirection.

Revise the check back to check csum_start directly, and only if
CHECKSUM_PARTIAL. Do leave the check in the updated location.
Check field regardless of whether TUNNEL_CSUM is configured.

Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
Link: https://lore.kernel.org/all/[email protected]/T/#u
Fixes: 8a0ed25 ("ip_gre: validate csum_start only on pull")
Reported-by: syzbot <[email protected]>
Signed-off-by: Willem de Bruijn <[email protected]>
Reviewed-by: Eric Dumazet <[email protected]>
Reviewed-by: Alexander Duyck <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Jakub Kicinski <[email protected]>
  • Loading branch information
wdebruij authored and kuba-moo committed Jun 9, 2022
1 parent d5d4c36 commit 8d21e99
Showing 1 changed file with 5 additions and 6 deletions.
11 changes: 5 additions & 6 deletions net/ipv4/ip_gre.c
Original file line number Diff line number Diff line change
Expand Up @@ -629,21 +629,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, pull_len);
skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
skb_reset_mac_header(skb);

if (skb->ip_summed == CHECKSUM_PARTIAL &&
skb_checksum_start(skb) < skb->data)
goto free_skb;
} else {
if (skb_cow_head(skb, dev->needed_headroom))
goto free_skb;
Expand Down

0 comments on commit 8d21e99

Please sign in to comment.