ipsec: Clear pad bytes in PF_KEY messages

[markj]

Summary

Various handlers for SADB messages will allocate a new mbuf and populate
some structures in it. Some of these structures, such as
struct sadb_supported, contain reserved fields that are not initialized
and are thus leaked to userspace.

Fix the problem by adding a helper to allocate zeroed mbufs. This
reduces code duplication and the overhead of zeroing these messages
isn't harmful.

Reported by: KMSAN
Sponsored by: The FreeBSD Foundation

[zlei]

Looks good to me.

[melifaro]

LGTM.
It would be nice to add a test for the basic PF_KEY functionality (for example, add 1 SA entry and list it afterwards)

sys/netipsec/key.c

833

Nit: worth considering using m_getm2() to simplify the logic here?

[markj]

Use m_get2() instead.

[markj]

In D38068#865012, @melifaro wrote:

LGTM.
It would be nice to add a test for the basic PF_KEY functionality (for example, add 1 SA entry and list it afterwards)

Ok, I will give it a try. Though, this is indirectly tested already by the sys/netipsec tests' use of setkey, which is how I found the bug.

[melifaro]

In D38068#865044, @markj wrote:

In D38068#865012, @melifaro wrote:

LGTM.
It would be nice to add a test for the basic PF_KEY functionality (for example, add 1 SA entry and list it afterwards)

Ok, I will give it a try. Though, this is indirectly tested already by the sys/netipsec tests' use of setkey, which is how I found the bug.

Ack, then it's not necessary. Did any of the tests explicitly failed?

[markj]

In D38068#865079, @melifaro wrote:

In D38068#865044, @markj wrote:

In D38068#865012, @melifaro wrote:

LGTM.
It would be nice to add a test for the basic PF_KEY functionality (for example, add 1 SA entry and list it afterwards)

Ok, I will give it a try. Though, this is indirectly tested already by the sys/netipsec tests' use of setkey, which is how I found the bug.

Ack, then it's not necessary. Did any of the tests explicitly failed?

Sort of. :) The problem was found by running tests in a kernel with KMSAN enabled: https://ci.freebsd.org/job/FreeBSD-main-amd64-KMSAN_test/

By default, a KMSAN report causes a kernel crash, which is what happens in this case. I'm going through test suite failures (i.e., crashes) at the moment and fixing bugs.

[melifaro]

In D38068#865080, @markj wrote:

In D38068#865079, @melifaro wrote:

In D38068#865044, @markj wrote:

In D38068#865012, @melifaro wrote:

LGTM.
It would be nice to add a test for the basic PF_KEY functionality (for example, add 1 SA entry and list it afterwards)

Ok, I will give it a try. Though, this is indirectly tested already by the sys/netipsec tests' use of setkey, which is how I found the bug.

Ack, then it's not necessary. Did any of the tests explicitly failed?

Sort of. :) The problem was found by running tests in a kernel with KMSAN enabled: https://ci.freebsd.org/job/FreeBSD-main-amd64-KMSAN_test/

By default, a KMSAN report causes a kernel crash, which is what happens in this case. I'm going through test suite failures (i.e., crashes) at the moment and fixing bugs.

Got it. So if we already have an automated way to check for the padding issues shat should be enough.

[markj]

In D38068#865081, @melifaro wrote:

In D38068#865080, @markj wrote:

In D38068#865079, @melifaro wrote:

In D38068#865044, @markj wrote:

In D38068#865012, @melifaro wrote:

LGTM.
It would be nice to add a test for the basic PF_KEY functionality (for example, add 1 SA entry and list it afterwards)

Ok, I will give it a try. Though, this is indirectly tested already by the sys/netipsec tests' use of setkey, which is how I found the bug.

Ack, then it's not necessary. Did any of the tests explicitly failed?

Sort of. :) The problem was found by running tests in a kernel with KMSAN enabled: https://ci.freebsd.org/job/FreeBSD-main-amd64-KMSAN_test/

By default, a KMSAN report causes a kernel crash, which is what happens in this case. I'm going through test suite failures (i.e., crashes) at the moment and fixing bugs.

Got it. So if we already have an automated way to check for the padding issues shat should be enough.

I do think it would be useful to have some comprehensive PF_KEY tests; I'm not sure how much of that surface is actually covered by the existing tests. But that would take some effort to do properly.