netinet6: ip6_setpktopt() requires NET_EPOCH

[kp]

Summary

ip6_setpktopt() can call ifnet_byindex() which requires epoch. Mark the
function as requiring NET_EPOCH, and ensure we enter it priot to calling
it.

Reported-by: [e-mail]
Sponsored by: Rubicon Communications, LLC ("Netgate")

[glebius]

I'm not IPv6 expert, so may suggest a stupid thing. There multiple places where we do ifnet_byindex() and then supply the result to in6_setscope(). Note that in6_setscope() enters the epoch recursively. Is it worth to create KPI that would setscope by index?

[kp]

In D33462#756972, @glebius wrote:

I'm not IPv6 expert, so may suggest a stupid thing. There multiple places where we do ifnet_byindex() and then supply the result to in6_setscope(). Note that in6_setscope() enters the epoch recursively. Is it worth to create KPI that would setscope by index?

Maybe. While I was looking at this I wondered if it'd be worth expanding the NET_EPOCH coverage a bit more so that basically the entire setsockopt/ioctl/... code path is covered. We've got a struct ifnet pointer if nothing else, and we shouldn't expect that to remain while we're outside of NET_EPOCH. That's a much bigger change though, and I was only looking to fix a low-hanging bug found by syzcaller.

[glebius]

In D33462#757218, @kp wrote:

Maybe. While I was looking at this I wondered if it'd be worth expanding the NET_EPOCH coverage a bit more so that basically the entire setsockopt/ioctl/... code path is covered. We've got a struct ifnet pointer if nothing else, and we shouldn't expect that to remain while we're outside of NET_EPOCH. That's a much bigger change though, and I was only looking to fix a low-hanging bug found by syzcaller.

But that would require to have memory preallocated, as many setsockopts allocate. The setsockopt path is probably the most difficult with the epoch protected network stack.

[kp]

In D33462#757630, @glebius wrote:

In D33462#757218, @kp wrote:

Maybe. While I was looking at this I wondered if it'd be worth expanding the NET_EPOCH coverage a bit more so that basically the entire setsockopt/ioctl/... code path is covered. We've got a struct ifnet pointer if nothing else, and we shouldn't expect that to remain while we're outside of NET_EPOCH. That's a much bigger change though, and I was only looking to fix a low-hanging bug found by syzcaller.

But that would require to have memory preallocated, as many setsockopts allocate. The setsockopt path is probably the most difficult with the epoch protected network stack.

Yeah, that's a concern.

I've had a quick look around w.r.t. your original suggestion, and as far as I can see there's not really a pattern of ifnet_byindex() / in6_setscope() in the tree, so introducing a separate function for this seems like it would accomplish little.

There might be room for a variant of in6_setscope() which assumes it's already in NET_EPOCH. There are certainly a few paths where that's the case and we could save ourselves the work of entering NET_EPOCH recursively. That's a bigger change (and with a bit more risk) than I'm looking to make here though.

[glebius]

In D33462#757723, @kp wrote:

I've had a quick look around w.r.t. your original suggestion, and as far as I can see there's not really a pattern of ifnet_byindex() / in6_setscope() in the tree, so introducing a separate function for this seems like it would accomplish little.
There might be room for a variant of in6_setscope() which assumes it's already in NET_EPOCH. There are certainly a few paths where that's the case and we could save ourselves the work of entering NET_EPOCH recursively. That's a bigger change (and with a bit more risk) than I'm looking to make here though.

Sorry for stupid suggestion then! The current patch looks good to me. A more thorough redesign can be left for later.