Nikolaos Gkarlis <nickgarlis@xxxxxxxxx> wrote:
> The kernel allows netlink messages that use the legacy NFT_CT_SRC and
> NFT_CT_DST keys in inet tables without an accompanying nft_meta
> expression specifying NFT_META_NFPROTO. This results in ambiguous
> conntrack expressions that cannot be reliably evaluated during packet
> processing.
>
> When that happens, the register size calculation defaults to IPv6 (16
> bytes) regardless of the actual packet family.
>
> This causes two issues:
> 1. For IPv4 packets, only 4 bytes contain valid address data while 12
> bytes contain uninitialized memory during comparison.
I don't think so, they are zeroed out, see nf_ct_get_tuple();
init_conntrack copies the entire thing so I don't see how stack garbage
can be placed in struct nf_conn and thus I don't see how registers can
contains crap. Do they? If yes, please provide a bit more information
on how this happens (e.g. kmsan report or similar).
> 2. nft userspace cannot properly display these rules ([invalid type]).
Thats a userspace bug; userspace that makes use of NFT_CT_SRC/DST must
provide the dependency.
This is not the kernels job, the kernel only must make sure that we
can't crash or otherwise leak hidden state (e.g. kernel memory
contents).
> The bug is not reproducible through standard nft commands, which use
> NFT_CT_SRC_IP(6) and NFT_CT_DST_IP(6) keys when NFT_META_NFPROTO is
> not defined.
Thats because not all kernel releases have NFT_CT_DST_IP(6), they were
added later. Switching userspace will break compatibility with older
kernels. That said, this key was added in v4.17 so we could change
nftables now to always use NFT_CT_DST_IP(6) instead and avoid adding
the NFPROTO implicit dep for this case.
> Fix by adding a pointer to the parent nft_rule in nft_expr, allowing
> iteration over preceding expressions to ensure that an nft_meta with
> NFT_META_NFPROTO has been defined.
But why? As far as I can see nothing is broken.
We don't force dependencies for other expressions either, why make
an exception here?
I'm sorry that I did not mention this earlier; in v1 i assumed intent
was to zap unused code (but its still used by nft), hence i did not
mention the above.
> Adding a pointer from nft_expr to nft_rule may be controversial, but
> it was the only approach I could come up with that provides context
> about preceding expressions when validating an expression.
We should not force this unless not doing it causes crash or
other misbehaviour, such as leaking private kernel memory content.
> struct nft_expr {
> const struct nft_expr_ops *ops;
> + const struct nft_rule *rule;
> unsigned char data[]
> __attribute__((aligned(__alignof__(u64))));
Ouch, sorry, I think this is a non-starter, nft_expr
should be kept as small as possible.
That said, I don't see why its necessary to add this pointer here,
it could be provided via nft_ctx for example.
