[PATCH nf] netfilter: nft_payload: rebuild vlan header on h_proto access

[PATCH nf] netfilter: nft_payload: rebuild vlan header on h_proto access

[Florian Westphal]

nft can perform merging of adjacent payload requests.
This means that:

ether saddr 00:11 ... ether type 8021ad ...

is a single payload expression, for 8 bytes, starting at the
ethernet source offset.

Check that offset+length is fully within the source/destination mac
addersses.

This bug prevents 'ether type' from matching the correct h_proto in case
vlan tag got stripped.

Fixes: de6843be3082 ("netfilter: nft_payload: rebuild vlan header when needed")
Reported-by: David Ward <david.ward@ll.mit.edu>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nft_payload.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
index 8cb800989947..120f6d395b98 100644
--- a/net/netfilter/nft_payload.c
+++ b/net/netfilter/nft_payload.c
@@ -154,6 +154,17 @@ int nft_payload_inner_offset(const struct nft_pktinfo *pkt)
 	return pkt->inneroff;
 }
 
+static bool nft_payload_need_vlan_copy(const struct nft_payload *priv)
+{
+	unsigned int len = priv->offset + priv->len;
+
+	/* data past ether src/dst requested, copy needed */
+	if (len > offsetof(struct ethhdr, h_proto))
+		return true;
+
+	return false;
+}
+
 void nft_payload_eval(const struct nft_expr *expr,
 		      struct nft_regs *regs,
 		      const struct nft_pktinfo *pkt)
@@ -172,7 +183,7 @@ void nft_payload_eval(const struct nft_expr *expr,
 			goto err;
 
 		if (skb_vlan_tag_present(skb) &&
-		    priv->offset >= offsetof(struct ethhdr, h_proto)) {
+		    nft_payload_need_vlan_copy(priv)) {
 			if (!nft_payload_copy_vlan(dest, skb,
 						   priv->offset, priv->len))
 				goto err;
-- 
2.41.0

Re: [PATCH nf] netfilter: nft_payload: rebuild vlan header on h_proto access

[Pablo Neira Ayuso]

On Fri, Sep 29, 2023 at 10:42:10AM +0200, Florian Westphal wrote:
> nft can perform merging of adjacent payload requests.
> This means that:
> 
> ether saddr 00:11 ... ether type 8021ad ...
> 
> is a single payload expression, for 8 bytes, starting at the
> ethernet source offset.
> 
> Check that offset+length is fully within the source/destination mac
> addersses.
> 
> This bug prevents 'ether type' from matching the correct h_proto in case
> vlan tag got stripped.

Patch LGTM, thanks for fixing my bug.

> Fixes: de6843be3082 ("netfilter: nft_payload: rebuild vlan header when needed")
> Reported-by: David Ward <david.ward@ll.mit.edu>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nft_payload.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nft_payload.c b/net/netfilter/nft_payload.c
> index 8cb800989947..120f6d395b98 100644
> --- a/net/netfilter/nft_payload.c
> +++ b/net/netfilter/nft_payload.c
> @@ -154,6 +154,17 @@ int nft_payload_inner_offset(const struct nft_pktinfo *pkt)
>  	return pkt->inneroff;
>  }
>  
> +static bool nft_payload_need_vlan_copy(const struct nft_payload *priv)
> +{
> +	unsigned int len = priv->offset + priv->len;
> +
> +	/* data past ether src/dst requested, copy needed */
> +	if (len > offsetof(struct ethhdr, h_proto))
> +		return true;
> +
> +	return false;
> +}
> +
>  void nft_payload_eval(const struct nft_expr *expr,
>  		      struct nft_regs *regs,
>  		      const struct nft_pktinfo *pkt)
> @@ -172,7 +183,7 @@ void nft_payload_eval(const struct nft_expr *expr,
>  			goto err;
>  
>  		if (skb_vlan_tag_present(skb) &&
> -		    priv->offset >= offsetof(struct ethhdr, h_proto)) {
> +		    nft_payload_need_vlan_copy(priv)) {
>  			if (!nft_payload_copy_vlan(dest, skb,
>  						   priv->offset, priv->len))
>  				goto err;
> -- 
> 2.41.0
>