Re: RFC: a checked mbuf accessor for -fbounds-safety

On 16 Sep 2026, at 00:29, Abhijeet Sharma <abhijeetsharma2002@gmail.com> wrote:
> 
> Hi,
> 
> I am adopting Clang's -fbounds-safety in the kernel TCP/IP stack under a
> FreeBSD Foundation project, with rpaulo@ as technical monitor.  The
> groundwork is in review as D58983 through D58987(M2), four of five accepted,
> adding the annotation vocabulary to sys/cdefs.h, a per-file opt-in to
> kern.mk that is empty by default, and a soft-trap runtime that counts a
> failed check and lets the kernel continue instead of panicking.  The access
> itself still happens, so bring-up gets a count and a backtrace rather than a
> mitigation.  Hard mode is the switch for that later.  Toolchain recipe at
> https://wiki.freebsd.org/BoundsSafety.
> 
> None of that touches packet data, which is the next step, so I would like
> to agree the interface before sending the patch.
> 
> mtod() is a cast of m_data, and no sibling field carries the storage bound.
> m_len is a sibling, so __sized_by(m_len) is expressible, but it describes
> the valid data rather than the storage.  The storage bound lives in
> M_START() and M_SIZE(), which are conditionals over m_flags, so no
> attribute can state it.  M2 marked m_data __unsafe_indexable, so all
> 2231 mtod() call sites produce unchecked pointers, and annotating netinet
> on top of that would check the parameters while leaving every packet read
> unchecked.  xnu hit the same wall and rebuilt the pointer through an
> accessor that reattaches the storage bound.  This follows that shape.
> 
> Visible only under __has_ptrcheck, so a file compiled without the flag sees
> today's macros unchanged.
> 
>    static __inline __pure caddr_t __header_bidi_indexable
>    m_mtod_current(const struct mbuf *m)
>    {
>     caddr_t __unsafe_indexable start;
>     __uintptr_t base, cur;
>     size_t size;
> 
>     start = __DECONST(caddr_t __unsafe_indexable, M_START(m));
>     if (__predict_false(start == NULL)) /* M_EXTPG: no mapped data */
>     return (__unsafe_forge_bidi_indexable(caddr_t, m->m_data, 0));
>     size = M_SIZE(m);
>     base = (__uintptr_t)start;
>     cur = (__uintptr_t)m->m_data;
>     if (__predict_false(size > __UINTPTR_MAX__ - base || cur < base ||
>        cur - base > size))
>     return (__unsafe_forge_bidi_indexable(caddr_t, m->m_data, 0));
>     return (__unsafe_forge_bidi_indexable(caddr_t, start, size) +
>        (cur - base));
>    }
> 
>    static __inline __pure caddr_t __header_bidi_indexable
>    m_mtod_len(const struct mbuf *m)
>    {
>     caddr_t __header_bidi_indexable storage;
>     size_t len;
>     caddr_t __sized_by(len) data;
> 
>     if (__predict_false((m->m_flags & M_EXTPG) != 0))
>     return (__unsafe_forge_bidi_indexable(caddr_t, m->m_data, 0));
>     storage = m_mtod_current(m);
>     len = m->m_len > 0 ? (size_t)m->m_len : 0;
>     data = storage; /* checked narrowing */
>     return (data);
>    }
> 
>    #define mtod(m, t)      ((t)(void *)m_mtod_current(m))
>    #define mtod_len(m, t)  ((t)(void *)m_mtod_len(m))
> 
> mtod() is bounded by the storage, not the valid data, because senders build
> into the space past m_len and extend it only afterwards.  tcp_respond()
> copies a header into mtod(m) while m_len is still 0, at tcp_subr.c:1798,
> with the tree's own comment there saying m_len is set later.  tcp_output()
> writes the payload at mtod(m) + hdrlen while m_len is hdrlen, at
> tcp_output.c:1081, and bumps m_len on the line after.  syncache_respond()
> appends options at the m_len boundary, at tcp_syncache.c:1980, and bumps
> m_len four lines later.  M_TRAILINGSPACE exists to size that kind of write
> and netinet and netinet6 have 25 uses of it.  A bound of [m_data, m_data +
> m_len) traps all of these, so it cannot be the default.  The storage bound
> leaves them working, still traps a walk off the end of the cluster, and
> gives every existing call site that check without being touched.
> 
> The containment test in m_mtod_current() is the one m_sanity() already runs
> in uipc_mbuf.c.  An inline header function cannot call it, so the check is
> open coded, but the predicate is the tree's rather than mine.
> 
> mtod_len() is the tighter view, for parsers, and it does not forge a bound
> of its own.  It narrows the storage view through a __sized_by(len)
> assignment, so the compiler checks m_len against the storage and an m_len
> larger than the cluster, or larger than what remains after m_data has
> advanced, traps at the narrowing.  A short pullup then traps at the first
> byte past the pulled-up data rather than only once the read leaves the
> cluster.
> 
> mtodo() is the offset form of the same cast and takes the same treatment,
> as m_mtod_current(m) + (o).  That bounds its 155 uses in the tree, 12 of
> them in netinet and netinet6.  Bounding mtod() and leaving mtodo() alone
> would keep the m_pulldown consumers unchecked, which is most of what reads
> a header out of a chain.
> 
> An mbuf whose m_data does not address its own storage gets no extent, so
> mtod() traps on the first dereference, and mtod_len() traps before that, at
> the narrowing, for any m_len above 0.  The metadata cannot distinguish a
> corrupted mbuf from the borrowed on-stack mbufs that bpf_mtap2() and
> ether_vlan_mtap() build around a caller's buffer, and bounding those by
> m_len would hand an attacker-influenced m_data a fresh bound unrelated to
> any real object.  Those builders should declare the borrowed buffer
> instead, and the tree already has the representation.  pfil_fake_mbuf()
> uses m_init() with M_NOFREE and m_extadd() with EXT_RXRING, which sets
> ext_buf and ext_size with no free or refcount semantics.  I converted the
> three sites to that form.  The knob-off kernel builds, a converted kernel
> boots, and tcpdump on lo0 put 942 packets through bpf_mtap2()'s fake mbuf
> with nothing dropped and no violations.  M_EXTPG mbufs get a zero extent
> from both accessors, so a dereference traps while the integer conversion
> the TLS code relies on keeps working.
> 
> The bound is forged here rather than declared, which is where this departs
> from xnu.  Their inline buffers are real arrays and their ext_buf carries
> __counted_by(ext_size).  Ours are m_dat[0] and m_pktdat[0], zero-length,
> and ext_buf's count sits across an anonymous union the compiler will not
> pair through.  So the unsafe conversion is concentrated in one function,
> m_data stays a pointer rather than becoming an integer as in xnu, and no
> m_data arithmetic in the tree changes.  struct tcpopt, tcp_addoptions() and
> tcp_respond() keep their layout and signatures, which matters because RACK
> and BBR are modules that share them.
> 
> A GENERIC amd64 kernel with tcp_sack.c and tcp_reass.c instrumented builds
> and boots, tcp_sack.c needing no changes and tcp_reass.c two __single
> locals, with the knob-off build byte-identical.  A test hook drives the
> accessors on a real m_getcl() mbuf across nine cases, including m_len
> beyond the capacity, m_data outside its storage, and M_EXTPG.  All nine
> behave as described.
> 
> No file in the series calls mtod() yet, so nothing there exercises the
> accessor under traffic.  ip_options.c is the nearest candidate, and the
> netinet suites on both architectures are still to come.
> 
> Happy to hear your thoughts, and happy to iterate on the design.

I am very concerned about the amount of churn and annotations these
diffs are already creating for just some of the basic groundwork, and
the ways in which ISO C constructs that are explicitly stated in the
spec to be semantically identical are treated as not being the same
under -fbounds-safety. I think there needs to be some very serious
consideration within the project as to whether this is something we
want before any of it lands, as it’s clear this is going to be a big
burden to carry. This looks like it has the potential to be much more
far reaching than something like CHERI, where we’ve tried very hard to
not break ISO C semantics except where absolutely necessary, and so I
find it disappointing to see others not following that attitude in
their C variants.

As a technical exploration project to understand how feasible this is
and how much churn is required it seems completely fine, and can
influence future project technical directions, but I do not think we
should be landing it for real in-tree before we know that.

Jessica