Audit explicit bounds enforcement for mbufs. #1980

qwattash · 2024-01-23T12:31:32Z

Remove some explicit cheri_setbounds that are redundant with sub-object bounds, replace with an assertion.
Use the new macro PHYS_TO_DMAP_LEN.
Promote some cheri_setbounds to cheri_setboundsexact, there are some additional cases with ext_buf that would be nice to check for exact representability. From my understanding, it is not currently always the case that the buffers are representable; in particular sys/net/iflib.c has some code that uses buffer space from an array and I suspect we can not assume it is always representable, however I did not yet dig much into how it is constructed. I marked a couple of these cases with a comment for now.

sys/kern/uipc_mbuf.c

brooksdavis · 2024-01-23T17:10:48Z

sys/kern/kern_mbuf.c

@@ -1543,6 +1543,7 @@ m_extadd(struct mbuf *mb, char *buf, u_int size, m_ext_free_t freef,
 	KASSERT(type != EXT_CLUSTER, ("%s: EXT_CLUSTER not allowed", __func__));

 	mb->m_flags |= (M_EXT | flags);
+	/* XXX-AM: Should we have an option to warn when these are not exact? */


Is it common that size != cheri_getlen(buf) in these cases?

You can have arbitrary buffers here in theory. I just used this function yesterday to attach a 4K buffer to an mbuf in the NVMe Fabrics target code. I also use it in the fabrics target code for I/O buffers from a LUN backend sent back over the socket. In theory those can be sized as an arbitrary number of blocks (e.g. N * 512). If N is a power of 2 those would all be fine, but possibly it could be a non-power-of-2 in which case you could end up with an inexact size.

bsdjhb · 2024-01-24T16:48:02Z

sys/kern/kern_mbuf.c

@@ -1543,6 +1543,7 @@ m_extadd(struct mbuf *mb, char *buf, u_int size, m_ext_free_t freef,
 	KASSERT(type != EXT_CLUSTER, ("%s: EXT_CLUSTER not allowed", __func__));

 	mb->m_flags |= (M_EXT | flags);
+	/* XXX-AM: Should we have an option to warn when these are not exact? */


You can have arbitrary buffers here in theory. I just used this function yesterday to attach a 4K buffer to an mbuf in the NVMe Fabrics target code. I also use it in the fabrics target code for I/O buffers from a LUN backend sent back over the socket. In theory those can be sized as an arbitrary number of blocks (e.g. N * 512). If N is a power of 2 those would all be fine, but possibly it could be a non-power-of-2 in which case you could end up with an inexact size.

sys/kern/uipc_mbuf.c

bsdjhb · 2024-01-24T16:52:11Z

sys/sys/mbuf.h

@@ -419,7 +419,8 @@ struct mbuf {
 				char		m_pktdat[0];
 			};
 		};
-		char	m_dat[0] __no_subobject_bounds;	/* !M_PKTHDR, !M_EXT */
+		/* !M_PKTHDR, !M_EXT */
+		char	m_dat[0] __subobject_use_remaining_size;


Ah, I think we certainly need a fair bit more explanation in the log of what we are depending on the compiler doing for m_dat and m_pktdat.

It occurs to me that I'm not sure whether the annotation is even necessary at this point. It could be a remnant of older compiler versions that did not know how to detect variable-sized arrays in certain situations. I need to check that, however I agree on the need for more context, I'll split these things into separate commits.

It looks like this annotation is still required, while m_pktdat does not require the annotation. This is quite strange and possibly a compiler bug. I kept this in a separate commit, hopefully I can drop it if I find out what is going on in LLVM.

Yeah definitely sounds like a compiler bug. Could you file a LLVM issue?

bsdjhb · 2024-01-24T16:54:50Z

sys/sys/mbuf.h

@@ -924,6 +925,7 @@ m_extaddref(struct mbuf *m, char *buf, u_int size, u_int *ref_cnt,

 	atomic_add_int(ref_cnt, 1);
 	m->m_flags |= M_EXT;
+	/* XXX-AM: Should we have an option to warn when these are not exact? */


These can also be arbitrary buffers. I'm not sure the caller is going to be able to do anything though if it is not exact. For example, the cxgbe(4) driver uses this for a feature it calls buffer packing where it allocates a large (multi-page) receive buffer and the NIC packs multiple received packets into the same large buffer. The driver allocates mbufs that then use the large buffer as the backing store but only reference the subset of the buffer. You can certainly use this with 9k jumbo frames where the resulting size passed in size might not be exactly representable?

I wonder if what we actually want is a counter for inexact bounds?

I wonder whether we want one specific for mbufs or a general one. I provisionally added a counter specifically for inexact ext_buf bounds.

bsdjhb · 2024-01-24T16:57:01Z

sys/sys/mbuf.h

@@ -419,7 +419,8 @@ struct mbuf {
 				char		m_pktdat[0];


So why does this not need __subobject_use_remaining_size?

Should be implicit for any variable-length fields ([0] is treated as variable)

Asserts that the length of a capability matches the expected length.

When the mbuf m_data is set to m_pktdat we rely on sub-object bounds to ensure that the resulting capability has length == MHLEN. The pure-capability kernel without sub-object bounds will ignore these cases.

When mbuf m_data is set to m_dat, rely on the compiler-generated sub-object bounds to ensure that the resulting capability has length == MLEN. The pure-capability kernel without sub-object bounds will ignore these cases.

Use the PHYS_TO_DMAP_LEN macro in m_apply_extpg_one() instead of explicitly setting bounds on the direct map capability.

In most cases, it is not possible to ensure exact bounds for m_ext.

This is required by the compiler for now.

The sysctl security.cheri.mbuf_imprecise_extbuf counts the number of times a non-representable capability is used as the buffer for an mbuf.m_ext.ext_buf.

qwattash requested review from markjdb and jrtc27 January 23, 2024 12:31

qwattash commented Jan 23, 2024

View reviewed changes

sys/kern/uipc_mbuf.c Outdated Show resolved Hide resolved

brooksdavis reviewed Jan 23, 2024

View reviewed changes

bsdjhb reviewed Jan 24, 2024

View reviewed changes

qwattash force-pushed the audit-mbuf-setbounds branch from d1ccadd to 5ae215e Compare February 9, 2024 16:21

qwattash marked this pull request as ready for review February 14, 2024 14:51

qwattash added 7 commits February 15, 2024 10:03

Add KASSERT_SUBOBJECT_BOUNDS to check compiler-generated bounds.

1ba2d16

Asserts that the length of a capability matches the expected length.

Rely on the compiler to produce sub-object bounds for mbuf m_pktdat.

11dd8eb

When the mbuf m_data is set to m_pktdat we rely on sub-object bounds to ensure that the resulting capability has length == MHLEN. The pure-capability kernel without sub-object bounds will ignore these cases.

Rely on compiler sub-object bounds for mbuf m_dat bounds.

460a55d

When mbuf m_data is set to m_dat, rely on the compiler-generated sub-object bounds to ensure that the resulting capability has length == MLEN. The pure-capability kernel without sub-object bounds will ignore these cases.

Use the PHYS_TO_DMAP_LEN macro in uipc_mbuf.

6cb61fd

Use the PHYS_TO_DMAP_LEN macro in m_apply_extpg_one() instead of explicitly setting bounds on the direct map capability.

Audit exact bounds for mbuf m_ext.ext_buf.

7668396

In most cases, it is not possible to ensure exact bounds for m_ext.

Fix mbuf m_dat sub-object bounds opt-out annotation.

04ed233

This is required by the compiler for now.

Add counter and associated sysctl to detect imprecise mbuf buffers.

b1602b6

The sysctl security.cheri.mbuf_imprecise_extbuf counts the number of times a non-representable capability is used as the buffer for an mbuf.m_ext.ext_buf.

qwattash force-pushed the audit-mbuf-setbounds branch from e5ad7e3 to b1602b6 Compare February 15, 2024 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit explicit bounds enforcement for mbufs. #1980

Audit explicit bounds enforcement for mbufs. #1980

qwattash commented Jan 23, 2024

brooksdavis Jan 23, 2024

bsdjhb Jan 24, 2024

bsdjhb Jan 24, 2024

bsdjhb Jan 24, 2024

qwattash Jan 25, 2024

qwattash Feb 8, 2024

arichardson Feb 15, 2024

bsdjhb Jan 24, 2024

brooksdavis Jan 24, 2024

qwattash Feb 8, 2024

bsdjhb Jan 24, 2024

arichardson Feb 7, 2024

qwattash Feb 8, 2024

Audit explicit bounds enforcement for mbufs. #1980

Are you sure you want to change the base?

Audit explicit bounds enforcement for mbufs. #1980

Conversation

qwattash commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment