On Thu, 21 Nov 2019 22:00:46 +0100 Stefano Brivio <sbrivio@xxxxxxxxxx> wrote: > On Thu, 21 Nov 2019 21:41:13 +0100 > Florian Westphal <fw@xxxxxxxxx> wrote: > > > Yes, exactly, we should only reject what either > > 1. would crash kernel > > 2. makes obviously no sense (missing or contradiction attributes). > > > > anything more than that isn't needed. > > > > > We could opt to be stricter indeed, by checking that a single netlink > > > batch contains a corresponding number of start and end elements. This > > > can't be done by the insert function though, we don't have enough > > > context there. > > > > Yes. If such 'single element with no end interval' can't happen or > > won't cause any problems then no action is needed. > > Yeah, I don't expect that to cause any problem. I don't have a > kselftest or nft test for it, because that would require nft to send > invalid elements, so I only tested those two cases manually. The > nastiest thing I could come up with was start > end, and it's now > covered by: > > if (memcmp(start, end, > f->groups / NFT_PIPAPO_GROUPS_PER_BYTE) > 0) > return -EINVAL; > > while: > - start == end is allowed, explicitly handled below > - end without any previous start (somewhat) correctly maps to < 0 > to > end On the contrary, good that you mentioned this, I haven't been creative enough. If we allow a < 0 > start element and keep the start pointer set to NULL, this ends up being used as it is in ->walk(). Another problem I found is that on the sequence: - start element only passed to ->insert() - API frees the start element - end element is then passed to ->insert() we would end up with a dangling ->start pointer, which is again problematic on a number of operations including walk(). Fixed in v2 by adding explicit checks (and comments). -- Stefano
