tech-net archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: Overflow bugs in m_get &c.



> Date: Sat, 16 Jul 2022 20:33:38 -0000 (UTC)
> From: mlelstv%serpens.de@localhost (Michael van Elst)
> 
> riastradh%NetBSD.org@localhost (Taylor R Campbell) writes:
> 
> >  . If alignbytes + nbytes > MCLBYTES, extends m with MEXTMALLOC.
> 
> Traditionally mbufs exist, because they can be allocated and freed
> in the network interrupt and don't fragment memory. That's true for
> regular mbufs and clusters, but not for extended allocations.
> 
> Lots of limitations are implied, like "buffer is big enough for
> everything" and "buffers are sufficiently aligned", without checking
> these details for every operatioon. The code was created in a time
> when every cycle counted.

If this is a concern, it can be separated from device-specific
concerns without imposing a responsibility on drivers that,
empirically, they don't live up to.

- If a _driver_ has device-specific size limitations, it can express
  them directly:

	if (nbytes > DRIVER_MAXPKTBYTES)
		return ENOBUFS;
	m = m_get_n(M_WAIT, MT_DATA, alignbytes, nbytes);
	...

- If a _machine_ has size limitations, perhaps we should have a knob
  to reject a set of MEXTMALLOC calls -- maybe all of them, maybe all
  beyond a certain size, &c.  But malloc and mbuf work via pool_cache
  these days so the logic behind MEXTMALLOC is not much different from
  the logic behind m_get or m_clget anyway.

> If you keep that distinction but try to hide it, I would guess this
> mostly adds overhead without making the code safer.

What's your alternative proposal to systematically and confidently
eliminate this class of bugs exposing vulnerabilities to the network?

> >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).
> 
> 1024 would be strange, as most of the world asssumes that a cluster
> can store an Ethernet packet of ~1500 bytes plus headers.

sun2/include/param.h:#define MCLSHIFT   10


Home | Main Index | Thread Index | Old Index