tech-net archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Overflow bugs in m_get &c.
We've had a class of bugs that works like this:
1. Network controls a packet length, call it N.
2. Network driver calls m_get/MGET or m_gethrd/MGETHDR, returning an
mbuf with space for MLEN or MHLEN bytes.
3. If N > MLEN or MHLEN, the driver conditionally calls m_clget/MCLGET
to expand the space to MCLBYTES (typically 2048 but sometimes 1024
or 4096).
3. Network driver sets m->m_len = N and memcpies N bytes of data into
mtod(m, void *).
The bug is that step (3) is a buffer overrun when N > MCLBYTES. See,
e.g., NetBSD-SA2020-003
(https://ftp.netbsd.org/pub/NetBSD/security/advisories/NetBSD-SA2020-003.txt.asc).
The API puts responsibility on the driver to validate that the length
is within some software parameter -- a parameter which has nothing to
do with the driver or network stack itself. Some drivers do this, but
not all. Some drivers also use MEXTMALLOC when N > MCLBYTES.
I tentatively propose we eliminate this class of bugs -- and reduce a
good deal of code, and simplify the driver's responsibilities -- using
the following approach:
- New m_get_n(how, type, alignbytes, nbytes) replaces any code using
m_get/MGET and m_clget or MEXTMALLOC:
. Allocates an mbuf m using m_get(how, type).
. If alignbytes + nbytes > MLEN but alignbytes + nbytes <= MCLBYTES,
extends m with m_clget.
. If alignbytes + nbytes > MCLBYTES, extends m with MEXTMALLOC.
. Sets m->m_len = alignbytes + nbytes.
. Aligns m with m_adj(m, alignbytes).
. Returns m, or NULL if any arithmetic overflowed or allocation
failed.
In the end, if m != NULL, the caller now has an mbuf with space for
nbytes bytes starting at mtod(m, void *), which is alignbytes past
the beginning of the actual data buffer.
- New m_gethdr_n(how, type, alignbytes, nbytes) is similar, but to
replace m_gethdr/MGETHDR instead of m_get; uses m_gethdr and MHLEN
internally instead of m_get and MLEN, and aditionally sets
m->m_pkthdr.len to alignbytes + nbytes before m_adj.
With this proposal, the driver is responsible only for verifying any
device-specific lengths -- other than that it doesn't need to know
anything about the device-independent machine-dependent relations
between magic numbers like MLEN and MCLBYTES.
I have started drafting a change to apply this throughout the tree,
but I haven't finished yet -- the change is mostly mechanical but it's
still a good deal of work to audit. Thoughts?
Home |
Main Index |
Thread Index |
Old Index