Re: [PATCH net 07/12] netfilter: nft_payload: restrict checksum offsets to known values

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: nft_payload: restrict checksum offsets to known values

This restricts the NFT_PAYLOAD_CSUM_INET checksum offset for transport
header writes, both at rule load time in
nft_payload_validate_inet_csum_offset() and at packet time via the new
nft_payload_csum_th_write_ok() helper.  Previously the transport header
case accepted any csum_offset.

> diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
> index e315d35f73d4..70f70a65e327 100644
> --- a/net/netfilter/nft_payload.c
> +++ b/net/netfilter/nft_payload.c
> @@ -1008,11 +1008,13 @@ static bool nft_payload_validate_inet_csum_offset(const struct nft_ctx *ctx,
>  		if (priv->csum_flags) /* makes no sense, asks for "re-update" of L4 checksum */
>  			return false;
>  
> -		/* no further check here; offset can't be negative so bogus
> -		 * offsets can corrupt L4 or payload but not l3 headers.
> -		 * We already allow arbitrary l4/inner payload writes.
> -		 */
> -		return true;
> +		/* Validate csum_offset is one of the supported transport header checksums */
> +		if (priv->csum_offset == offsetof(struct tcphdr, check) ||
> +		    priv->csum_offset == offsetof(struct udphdr, check) ||
> +		    priv->csum_offset == offsetof(struct icmp6hdr, icmp6_cksum))
> +			return true;
> +
> +		return false;
>  	case NFT_PAYLOAD_INNER_HEADER:
>  		return true;
>  	case NFT_PAYLOAD_TUN_HEADER:
> @@ -1046,6 +1048,25 @@ static bool nft_payload_csum_nh_write_ok(const struct nft_payload_set *priv,
>  	return false;
>  }
>  
> +static bool nft_payload_csum_th_write_ok(const struct nft_payload_set *priv,
> +					 const struct nft_pktinfo *pkt)
> +{
> +	if (!(pkt->flags & NFT_PKTINFO_L4PROTO))
> +		return false;
> +
> +	switch (pkt->tprot) {
> +	case IPPROTO_TCP:
> +		return priv->csum_offset == offsetof(struct tcphdr, check);
> +	case IPPROTO_UDP:
> +	case IPPROTO_UDPLITE:
> +		return priv->csum_offset == offsetof(struct udphdr, check);
> +	case IPPROTO_ICMPV6:
> +		return priv->csum_offset == offsetof(struct icmp6hdr, icmp6_cksum);
> +	}
> +
> +	return false;
> +}

[Severity: High]
Should nft_payload_csum_th_write_ok() also handle IPPROTO_ICMP?  There is
no arm for it, and neither IPPROTO_ICMP nor struct icmphdr appear anywhere
in nft_payload.c (linux/icmp.h is not included), so pkt->tprot ==
IPPROTO_ICMP falls out of the switch and returns false.

At the same time the load time check above accepts csum_offset == 2,
because the two checksum fields sit at the same offset:

include/uapi/linux/icmp.h:
struct icmphdr {
  __u8		type;
  __u8		code;
  __sum16	checksum;
  ...

So a rule such as:

    nft add rule ip filter output icmp id set 1

still loads without error, but at packet time the checksum fixup is
skipped.  In nft_payload_set_eval() the helper is only the middle operand
of an && chain, so a false return does not reach the err label:

	if (priv->csum_type == NFT_PAYLOAD_CSUM_INET &&
	    nft_payload_csum_write_ok(pkt, priv) &&
	    nft_payload_csum_inet(skb, src, fsum, tsum, csum_offset))
		goto err;
	...
	if (skb_ensure_writable(skb, max(offset + priv->len, 0)) ||
	    skb_store_bits(skb, offset, src, priv->len) < 0)
		goto err;

Does this mean the ICMPv4 header bytes are still rewritten by
skb_store_bits() while the old checksum is left in place, so such packets
go out with a stale checksum?  Before this patch the
NFT_PAYLOAD_TRANSPORT_HEADER case of nft_payload_csum_write_ok() returned
true unconditionally, so previously working ICMPv4 mangling rules would
change behaviour here.

The accepted protocol list looks like the one in
nft_payload_l4csum_offset(), which serves the csum_flags pseudo-header
path and therefore has a reason to omit ICMPv4.  Is that the right list to
reuse for the NFT_PAYLOAD_CSUM_INET path?

Two things that may be worth considering: adding

	case IPPROTO_ICMP:
		return priv->csum_offset == offsetof(struct icmphdr, checksum);

to nft_payload_csum_th_write_ok(), and making the run time refusal set
NFT_BREAK rather than silently skipping the checksum update, so load time
and run time agree.

> @@ -1055,9 +1076,10 @@ static bool nft_payload_csum_write_ok(const struct nft_pktinfo *pkt,
>  	case NFT_PAYLOAD_NETWORK_HEADER:
>  		return nft_payload_csum_nh_write_ok(priv, pkt);
>  	case NFT_PAYLOAD_TRANSPORT_HEADER:
> +		return nft_payload_csum_th_write_ok(priv, pkt);
>  	case NFT_PAYLOAD_INNER_HEADER:
> -		/* neither offsets are validated, offsets cannot be
> -		 * negative so real l3 headers cannot be mangled.
> +		/* offset is not validated, offset cannot be
> +		 * negative so real l3/l4 headers cannot be mangled.
>  		 */
>  		return true;
>  	case NFT_PAYLOAD_TUN_HEADER:

[ ... ]