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.
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;
| }
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.
Cheers, Phil
