Florian Westphal <fw@xxxxxxxxx> wrote:
> 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.
Yes they are zeroed out, sorry for the confusion. I haven't observed any
leaks. What I meant was that if you manage to create such rules from a
buggy userspace you get something like this:
table inet test-table {
chain test-chain {
type filter hook input priority filter; policy accept;
ct original saddr 0xc0000201000000000000000000000000 [invalid
type] counter packets 0 bytes 0
ct original saddr 0xc0000201 [invalid type] counter packets 0 bytes 0
}
}
In my test, the first rule contains c000:201:: and the second one
192.0.2.1.
When I send test packets with:
nping --tcp -p 80 --source-ip 192.0.2.1 127.0.0.1
I see the counters for both rules being increased suggesting that both
of them matched:
table inet test-table {
chain test-chain {
type filter hook input priority filter; policy accept;
ct original saddr 0xc0000201000000000000000000000000 [invalid
type] counter packets 3 bytes 120
ct original saddr 0xc0000201 [invalid type] counter packets 3 bytes 120
}
}
> Thats a userspace bug; userspace that makes use of NFT_CT_SRC/DST must
> provide the dependency.
Yes I guess that makes sense, garbage in, garbage out. I was just used
to seeing some kind of errno being returned from any other invalid input
and I assumed that it might have been a bug in the validation.
> But why? As far as I can see nothing is broken.
Honestly, I am not really sure whether it is an issue or not and this
was mostly driven by the assumption that the kernel shouldn't trust the
userspace to properly validate its input.
> We don't force dependencies for other expressions either, why make
> an exception here?
There is probably no strong enough reason to. Was the decision to not
force dependencies intentional or something that was left as a TODO?
> 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.
No problem, diving into this was mostly done for fun.
> Ouch, sorry, I think this is a non-starter, nft_expr
> should be kept as small as possible.
Yes, putting a pointer there was kinda silly.
Thanks for taking the time to explain. Let me know if you'd like any
more info about this or another patch involving nft_ctx instead.
Otherwise, I’m fine leaving this here.
