Re: [PATCH v14 nf-next 2/3] netfilter: bridge: Add conntrack double vlan and pppoe

[Florian Westphal <fw@strlen.de>]

Eric Woudstra <ericwouds@gmail.com> wrote:
> This adds the capability to conntrack 802.1ad, QinQ, PPPoE and PPPoE-in-Q
> packets that are passing a bridge, only when a conntrack zone is set.
> 
> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
> ---
>  net/bridge/netfilter/nf_conntrack_bridge.c | 88 ++++++++++++++++++----
>  1 file changed, 72 insertions(+), 16 deletions(-)
> 
> +			data_len = ntohs(ph->hdr.length) - 2;

Shouldn't there be some validation on data_len here?

> +		if (!pskb_may_pull(skb, offset + sizeof(struct iphdr)))
> +			goto do_not_track;
 
>  		len = skb_ip_totlen(skb);
> -		if (pskb_trim_rcsum(skb, len))
> -			return NF_ACCEPT;
> +		if (data_len < len)
> +			len = data_len;

Hmm.  So if ph->hdr.length is smaller than what ip header claims,
len shrinks.

If its higher, then the mismatch is ignored and we only use
the ip header length (i.e., the smaller value).

> +		if (pskb_trim_rcsum(skb, offset + len))
> +			goto do_not_track;

Is the intent that garbage data_len is caught here and

>  		if (nf_ct_br_ip_check(skb))
> -			return NF_ACCEPT;

here?  If so, maybe a comment could help.

> +		goto do_not_track;
>  	}
>  
> -	if (ret != NF_ACCEPT)
> -		return ret;
> +	if (ret == NF_ACCEPT)
> +		ret = nf_conntrack_in(skb, &bridge_state);
>  
> -	return nf_conntrack_in(skb, &bridge_state);
> +do_not_track:
> +	if (offset) {

if (ret == NF_ACCEPT && offset) { ...

Else skb could have been free'd. There should be test cases for this
functionality included.  If we lack test cases for the existing
functionality, which might be the case, please consider submitting
a reduced test case first so it can be applied regardless of the
remaining functionality.