opencrypto: assert that mbufs are writable

[kp]

Summary

Make it a little easier to detect if we've mistakenly passed a non-writable mbuf
to the crypto code.
This ought to make bugs like 280036 easier to find.

Sponsored by: Rubicon Communications, LLC ("Netgate")

[markj]

sys/opencrypto/cryptodev.h

537 ↗

(On Diff #143068)

I think this will raise false positives. It's possible for the crp to have separate input and output buffers. ktls_ocf_tls_cbc_encrypt() uses an mbuf as input and outputs to a different structure, for example, so the mbuf doesn't need to be writeable.

You could perhaps predicate this check on CRYPTO_HAS_OUTPUT_BUFFER(crp), but that would require some reshuffling of existing code.

You might instead put this assertion in crypto cursor routines which actually write to the mbuf, like crypto_cursor_copyback(). Or push it into the mbuf routines, m_copydata(), m_copyback().

[kp]

Move the assertions to crp_sanity() and cope with output buffers.

[kp]

sys/opencrypto/cryptodev.h

537 ↗

(On Diff #143068)

It would indeed be nice to also have such an assertion in m_copyback(). Doing so revealed that we violate this during at least some of the pf tests.
That's because https://cgit.freebsd.org/src/tree/sys/netinet6/ip6_forward.c#n148 makes the mbuf non-writable in some situations. So perhaps pf (and ipf and ipfw) ought to grow m_unshare() calls as well.

[markj]

This seems ok to me, but please make sure to run the ipsec and ktls tests as well. (For the latter you need to set kern.ipc.tls.enable=1 and run tests/sys/kern/ktls_test.)

sys/opencrypto/crypto.c

1295

IMHO it would be nice to use KASSERT() here, for consistency with the rest of the function.

[kp]

In D46572#1062624, @markj wrote:

This seems ok to me, but please make sure to run the ipsec and ktls tests as well. (For the latter you need to set kern.ipc.tls.enable=1 and run tests/sys/kern/ktls_test.)

That's a good suggestion, because the 'ktls_test:ktls_transmit_aes128_cbc_1_0_sha1_control' test panics with this assertion.
It seems to end up with an M_RDONLY | M_EXTPG mbuf, which fails the M_WRITABLE() assertion. I'm toying with adding an m_unshare in the relevant code, but that seems to panic too.

I'm sure I'm missing important details there, I'm not at all familiar with ktls.

[jhb]

You might try doing this again after my other series lands. In this case though I think you'd need M_WRITABLE_EXTPG.