Page MenuHomeFreeBSD

opencrypto: assert that mbufs are writable
AbandonedPublic

Authored by kp on Sep 7 2024, 9:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 1:30 AM
Unknown Object (File)
Thu, Nov 7, 12:34 PM
Unknown Object (File)
Thu, Oct 31, 9:11 PM
Unknown Object (File)
Thu, Oct 31, 11:54 AM
Unknown Object (File)
Fri, Oct 25, 9:34 AM
Unknown Object (File)
Thu, Oct 17, 11:32 AM
Unknown Object (File)
Thu, Oct 17, 6:07 AM
Unknown Object (File)
Oct 7 2024, 1:46 PM
Subscribers

Details

Reviewers
jhb
Group Reviewers
network
pfsense
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")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 59412
Build 56299: arc lint + arc unit

Event Timeline

kp requested review of this revision.Sep 7 2024, 9:33 AM
markj added inline comments.
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().

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

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.

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.

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.

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