Hi Phil,
On Fri, Jul 21, 2023 at 11:59:55AM +0200, Phil Sutter wrote:
> Pablo,
>
> On Mon, Jul 17, 2023 at 01:07:35PM +0200, Pablo Neira Ayuso wrote:
> > On Sat, Jul 15, 2023 at 02:59:26PM +0200, Phil Sutter wrote:
> > > When comparing matches for equality, trailing data in among match is not
> > > considered. Therefore two matches with identical pairs count may be
> > > treated as identical when the pairs actually differ.
> >
> > By "trailing data", you mean the right-hand side of this?
> >
> > fe:ed:ba:be:00:01=10.0.0.1
> >
> > > Matches' parsing callbacks have no access to the xtables_match itself,
> > > so can't update userspacesize field as needed.
> > >
> > > To fix this, extend struct nft_among_data by a hash field to contain a
> > > DJB hash of the trailing data.
> >
> > Is this DJB hash use subject to collisions?
>
> Thanks for the heads-up. I suspected DJB hash algo might not be perfect
> when it comes to collisions, but "good enough" for the task. In fact,
> collisions are pretty common, so this approach is not a proper solution
> to the problem.
>
> Searching for other ways to fix the issue, I noticed that
> compare_matches() was deliberately changed to compare only the first
> 'userspacesize' bytes of extensions to avoid false-negatives caused by
> kernel-internals in extension data.
Indeed, that was a deliberate decision.
> I see two different solutions and would like to hear your opinion. First
> one is a hack, special treatment for among match in compare_matches():
>
> | @@ -381,6 +381,7 @@ bool compare_matches(struct xtables_rule_match *mt1,
> | for (mp1 = mt1, mp2 = mt2; mp1 && mp2; mp1 = mp1->next, mp2 = mp2->next) {
> | struct xt_entry_match *m1 = mp1->match->m;
> | struct xt_entry_match *m2 = mp2->match->m;
> | + size_t cmplen = mp1->match->userspacesize;
> |
> | if (strcmp(m1->u.user.name, m2->u.user.name) != 0) {
> | DEBUGP("mismatching match name\n");
> | @@ -392,8 +393,10 @@ bool compare_matches(struct xtables_rule_match *mt1,
> | return false;
> | }
> |
> | - if (memcmp(m1->data, m2->data,
> | - mp1->match->userspacesize) != 0) {
> | + if (!strcmp(m1->u.user.name, "among"))
> | + cmplen = m1->u.match_size - sizeof(*m1);
> | +
> | + if (memcmp(m1->data, m2->data, cmplen) != 0) {
> | DEBUGP("mismatch match data\n");
> | return false;
> | }
This incremental update is relatively simple and it is only 'among'
that requires this special handling. Maybe you start with this, also
placing a comment to describe the intention for this particular case.
I don't remember if among allows to delete a rule with set elements
that are placed in different order.
Then, if you have to follow up because this is not enough...
> The second one is more generic, reusing extensions' 'udata' pointer. One
> could make xtables_option_{m,t}fcall() functions zero the scratch area
> if present (so locally created extensions match ones fetched from
> kernel) and compare that scratch area in compare_matches(). For among
> match, using the scratch area to store pairs is fine.
then pursue this second approach?
Thanks for explaining.
