In D53484#1223559, @ivy wrote:

In D53484#1223366, @glebius wrote:

Note that there are tests that use routed :(

oh, i missed that; i'll investigate and update the diff. thanks.

In D53484#1223413, @cy wrote:

Would there be any value into turning routed into a port? I don't use routed though I did in a different lifetime on a different O/S (Tru64).

there is already a net/freebsd-routed package, maintained by one "cy@FreeBSD.org" :-) this is mentioned in the UPDATING entry in this diff.

Would there be any value into turning routed into a port? I don't use routed though I did in a different lifetime on a different O/S (Tru64).

1. Make sure frentry_t passed from userland >= sizeof(frentry_t).

I do have something ready to upload to this review. I want to exercise it a bit more. There will be another review for a similar patch for ipnat, also being tested here.

Would it be better if we got together in a Teams call to discuss this?

I added jrm@ as a subscriber as Ed, Joseph and I had discussed this patch after the wireless meeting.

Removal of two size_t casts.

This is based on the misunderstanding that fr_names only contains interface names.

This is based on my incorrect understanding that fr_names contains only interface names.

fr_names appears to be used for more than interface names. Until I fully understand this, a review is pointless.

This will need some testing.

This review is still active.

The assumption that fr_names only contains interface names is incorrect and elements are not limited to LIFNAMSIZ. I need to step back to try an understand this.

Fix a typo.

Again, update interror.c. Need to save it too before updating the commit.

Validating fr_sifpidx in a separate commit is cumbersome. Both depend
on fr_namelen not to exceed the fr_names boundary.

It makes no sense to do this test in isolation. This test and D53276 require fp->fr_namelen not to exceed LIFNAMSIZ. The entire issue will be fixed by D53276 instead of splitting the fix into two.

Add missing ipferror.c updates.

This is no longer needed as the validation is now to be performed in frrequest().

I've taken Mark's suggestion to move the validation code from
ipf_synclist() to frrequest().

Hmm. I may want to abandon the reviews and take a broader approach to input verification. Some input verification is already there but needs improvement.

In D53278#1219408, @markj wrote:

Looking at the code, I think this structure is copied in in ipf_nat_ioctl(). There, we copy in a struct ipnat without the variable-length interface name after it. Then, we check the in_size field of the copied-in structure (which presumably includes the length of the interface name) and copy the whole thing in.

So, I guess we should handle validation there, and we should check that the string is in the bounds defined by the in_size field instead.

In D53276#1219432, @markj wrote:

Shouldn't the validation be done at the ioctl layer, i.e., in frrequest() when we're copying in the frentry object?

Use LIFNAMSIZ.

Move the test to frrequest() (on input).

Use new ipf_frname_vfy() function.

I really should have tested this before submitting the updated diff. I will test it now.

Fix the last of the issues Ilja Van Sprundel had identified in his
email regarding ip_htable.c.

In D53308#1217827, @emaste wrote:

Or if you wanted you could just use nitems() in find_error instead of IPF_NUM_ERRORS

After testing this patch, it needs more work.

In D53275#1217300, @markj wrote:

If you haven't tried testing ipf with the GENERIC-KMSAN kernel config, I suggest it: it'll automatically catch bugs of this kind. kmsan.9 has some details.

Fix stupid mistake.

Use strnlen().

Use strnlen() instead of strlen().

Use strnlen().

Use strnlen() instead of strlen.

I will review my use of strlen throughout. I'll get back to you tomorrow.

This is a duplicate.