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

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 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.

ip_nat doesn't use frrequest(). It's parsed from a different file. The ioctl handler passes ip_nat requests to ipf_nat*. Yeah I should do the same there. Would probably make more sense than playing whack-a-mole.

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.

Regarding reloading rules from ipfs, I'm not happy with ipfs. It should be exported as a set of text rules that can be parsed on re-input rather than data structures that can be abused. It should be removed as it has some panic issues and nobody uses it.