Hi Florian,
On Mon, Aug 01, 2022 at 03:56:25PM +0200, Florian Westphal wrote:
> v2:
> - fix a UAF during rule listing. When OP_AND gets culled,
> 'expr' is free'd as well ahead of time because they alias one
> another in the set key case (there is no compare/relational op).
> - add and handle plain 'vlan id' in a set key.
> in v1, this would be shown with the '& 0xfff' included, because
> v1 only removed OP_AND in concatenations.
>
> Eric Garver reported a number of issues when matching vlan headers:
>
> In: update @macset { ether saddr . vlan id timeout 5s }
> Out: update @macset { @ll,48,48 . @ll,112,16 & 0xfff timeout 5s }
>
> This is because of amnesia in nft during expression decoding:
> When we encounter 'vlan id', the L2 protocl (ethernet) is replaced by
> vlan, so we attempt to match @ll,48,48 vs. the vlan header and come up
> empty.
>
> The vlan decode fails because we can't handle '& 0xfff' in this
> instance, so we can locate the right offset but the payload expression
> length doesn't match the template length (16 vs 12 bits).
>
>
> The main patch is patch 3, which adds a stack of l2 protocols to track
> instead of only keeping the cumulative size.
>
> The latter is ok for serialization (we have the expression tree, so its
> enough to add the size of the 'previous' l2 headers to payload
> expressions that match the new 'top' l2 header.
>
> But for deserialization, we need to be able to search all protocols base
> headers seen.
>
> The remaining patches improve handling of 'integer base type'
> expressions and add test cases.
series LGTM.
A few more nits:
# cat test.nft
add table netdev x
add chain netdev x y
add rule netdev x y ip saddr 1.2.3.4 vlan id 10
# nft -f test.nft
test.nft:3:38-44: Error: conflicting protocols specified: ether vs. vlan
add rule netdev x y ip saddr 1.2.3.4 vlan id 10
^^^^^^^
it also occurs here:
# cat test.nft
add table netdev x
add chain netdev x y
add set netdev x macset { typeof ip saddr . vlan id; flags dynamic,timeout; }
add rule netdev x y update @macset { ip saddr . vlan id }
# nft -f test.nft
test.nft:4:49-55: Error: conflicting protocols specified: ether vs. vlan
add rule netdev x y update @macset { ip saddr . vlan id }
^^^^^^^
This is related to an implicit ether dependency.
If you see a way to fix this incrementally, I'm fine with you pushing
out this series and then you follow up.
Another issue: probably it would make sense to bail out when trying to
use 'vlan id' (and any other vlan fields) from ip/ip6/inet families?
vlan_do_receive() sets skb->dev to the vlan device, and the vlan
fields in the skbuff are cleared. In iptables, there is not vlan match
for this reason.
