pf: Fix tag hashing

[markj]

Summary

tagname2tag() hashes the tag name before truncating it to 63 characters.
tag_unref() removes the tag from the name hash by computing the hash
over the truncated name. Modify tagname2hashindex() to only hash over
the bytes that will actually be saved. I don't see any other problems
with permitting tag names longer than 63 characters, but maybe there's
some reason to prohibit them altogether.

Reported by: [e-mail]

Test Plan

Tested using a syzkaller reproducer. pf regression tests pass.

[kp]

Do you have a pointer to the syzcaller reproducer for this issue? I'd like to add a test.

[markj]

In D27346#610754, @kp wrote:

Do you have a pointer to the syzcaller reproducer for this issue? I'd like to add a test.

It's here: https://syzkaller.appspot.com/bug?id=dd302de76ce71516f929e4504daa4010f033cd9c

[kp]

Looks good to me.

I have a test case for it in D27350

[markj]

Looking at this again with fresh eyes, I think the real problem is that none of the pf ioctls verify that strings embedded in parameter structures are nul-terminated.

[kp]

In D27346#610905, @markj wrote:

Looking at this again with fresh eyes, I think the real problem is that none of the pf ioctls verify that strings embedded in parameter structures are nul-terminated.

Yeah, we should do what we already do for pr->anchor (in DIOCADDRULE) and explicitly zero-terminate it. This patch still makes sense though, even if we explicitly zero-terminate in the ioctl handler already. If only for robustness if we happen to forget a path.

This is all made that much harder by the fact that pf shares its internal data structures with the world via its ioctl interface. I've got in-progress work to separate that out, and that gives us a logical place (i.e. the function that translates ioctl data structures into pf kernel data structures) to do this sort of thing.
The struct pf_rule code is going to be the last I get round to doing, because it's so central and uses and is used by a number of other structures that also need to be changed.

[markj]

In D27346#610905, @markj wrote:

Looking at this again with fresh eyes, I think the real problem is that none of the pf ioctls verify that strings embedded in parameter structures are nul-terminated.

In D27346#610916, @kp wrote:

In D27346#610905, @markj wrote:

Looking at this again with fresh eyes, I think the real problem is that none of the pf ioctls verify that strings embedded in parameter structures are nul-terminated.

Yeah, we should do what we already do for pr->anchor (in DIOCADDRULE) and explicitly zero-terminate it. This patch still makes sense though, even if we explicitly zero-terminate in the ioctl handler already. If only for robustness if we happen to forget a path.

Fair enough. I'll note in the commit message that this is something of a workaround.

This is all made that much harder by the fact that pf shares its internal data structures with the world via its ioctl interface. I've got in-progress work to separate that out, and that gives us a logical place (i.e. the function that translates ioctl data structures into pf kernel data structures) to do this sort of thing.
The struct pf_rule code is going to be the last I get round to doing, because it's so central and uses and is used by a number of other structures that also need to be changed.

I started trying to do this myself, but wasn't sure about the best approach. I started adding explicit checks for nul-termination rather than doing nul-termination in the ioctl handlers, but that doesn't make sense for out-parameters, and I can't easily see whether a given string field is an out-parameter or not. If you've got plans in this area I'm happy to leave it to you. :)