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