pf: Validate user string nul-termination before copying

[markj]

Summary

Some pf ioctl handlers use strlcpy() to copy strings when converting
from user structures to their in-kernel representations. strlcpy()
ensures that the destination will be nul-terminated, but it assumes that
the source is nul-terminated. In particular, it returns the full length
of the source string, so if the source is not nul-terminated, strlcpy()
will keep scanning until it finds a nul byte, and it may encounter an
unmapped page first.

Add a helper to validate user strings and use it in ioctl handlers which
use strlcpy() to copy user-provided strings.

There are also many places where we look up a ruleset using a
user-provided anchor string. In some ioctl handlers we were already
nul-terminating the string, avoiding the same problem, but in other
places we were not. Fix those by nul-terminating as well. Aside from
being consistent, anchors have a maximum length of MAXPATHLEN - 1 so
calling strnlen() might not be so desirable, though I imagine that
anchors are usually short in practice.

Reported by: [e-mail]

[kp]

Thanks. I'd just figured this one out too (the [e-mail] reproducer actually reproduced the panic for me), but your solution is better than what I did (ensuring the incoming string is always 0-terminated).

I do wonder if we should have a pf_strlcpy() or something instead of pf_check_ustr(), so that we do the validation and copy in one call.

[markj]

In D31169#701512, @kp wrote:

Thanks. I'd just figured this one out too (the [e-mail] reproducer actually reproduced the panic for me), but your solution is better than what I did (ensuring the incoming string is always 0-terminated).

You mean, your solution was to always nul-terminate user strings? I did it this way to help catches cases where a user-specified string is too long and would be truncated if we silently accepted it. For anchors we were already nul-terminating the string in some places, so I just kept it that way.

I do wonder if we should have a pf_strlcpy() or something instead of pf_check_ustr(), so that we do the validation and copy in one call.

That's a good idea, I'll try implementing that.

[kp]

In D31169#701653, @markj wrote:

In D31169#701512, @kp wrote:

Thanks. I'd just figured this one out too (the [e-mail] reproducer actually reproduced the panic for me), but your solution is better than what I did (ensuring the incoming string is always 0-terminated).

You mean, your solution was to always nul-terminate user strings? I did it this way to help catches cases where a user-specified string is too long and would be truncated if we silently accepted it.

Yes. Your approach is better for exactly that reason.

[markj]

Add a pf_user_strcpy() helper which both checks the source length and
copies the string.