[PATCH net v2] ppp: enforce minimum MTU/MRU to fix Deflate buffer underflows

[PATCH net v2] ppp: enforce minimum MTU/MRU to fix Deflate buffer underflows

[Yilin Zhang]

z_compress() assumes that its output buffer can hold the six-byte
PPP/Deflate header.  A user with CAP_NET_ADMIN can set the MTU to 1,
making pad_compress_skb() allocate a five-byte skb.  z_compress() then
writes the header past the end and makes avail_out wrap when subtracting
the header length, allowing zlib to continue writing past the allocation.

On receive, PPPIOCSMRU accepts any signed int.  An MRU of -1 makes
ppp_decompress_frame() allocate a three-byte skb, after which
z_decompress() writes the PPP header and makes avail_out wrap when
subtracting PPP_HDRLEN, letting inflate overwrite the skb.

Set the PPP netdevice minimum MTU to 128 and reject MRU values below 128
(or large enough to overflow the allocation-size calculation) in
PPPIOCSMRU, matching the minimum that pppd uses when negotiating MRU
values.  This ensures that the output buffer seen by z_compress() and
z_decompress() always covers the PPP/Deflate header, so avail_out
cannot wrap.

Take one MTU snapshot when calculating both the skb allocation and
compressor sizes, so a concurrent MTU change cannot make them
inconsistent.

The two memory-corruption paths produce (Linux 6.1.0, KASAN, decoded):

  BUG: KASAN: slab-out-of-bounds in deflate_slow (lib/zlib_deflate/defutil.h:431 lib/zlib_deflate/deflate.c:1123)
  Write of size 8198 at addr ff110000045edc06 by task exp/78
  CPU: 1 PID: 78 Comm: exp Not tainted 6.1.0 #1
  Call Trace:
    kasan_check_range (mm/kasan/generic.c:190)
    memcpy (mm/kasan/shadow.c:65)
    deflate_slow (lib/zlib_deflate/defutil.h:431 lib/zlib_deflate/deflate.c:1123)
    zlib_deflate (lib/zlib_deflate/deflate.c:412)
    z_compress (drivers/net/ppp/ppp_deflate.c:227)
    __ppp_xmit_process (drivers/net/ppp/ppp_generic.c:1703 drivers/net/ppp/ppp_generic.c:1826 drivers/net/ppp/ppp_generic.c:1646)
    ppp_xmit_process (drivers/net/ppp/ppp_generic.c:1668)
    ppp_write (drivers/net/ppp/ppp_generic.c:520)

  Allocated by task 78:
    __alloc_skb (net/core/skbuff.c:437 net/core/skbuff.c:509)
    __ppp_xmit_process (include/linux/skbuff.h:1267 drivers/net/ppp/ppp_generic.c:1692 drivers/net/ppp/ppp_generic.c:1826 drivers/net/ppp/ppp_generic.c:1646)
    ppp_xmit_process (drivers/net/ppp/ppp_generic.c:1668)
    ppp_write (drivers/net/ppp/ppp_generic.c:520)

  The buggy address belongs to the object at ff110000045edc00
   which belongs to the cache kmalloc-512 of size 512
  The buggy address is located 6 bytes inside of
   512-byte region [ff110000045edc00, ff110000045ede00)

  BUG: KASAN: slab-out-of-bounds in zlib_inflate (lib/zlib_inflate/inflate.c:458)
  Write of size 8191 at addr ff110000021d1044 by task ksoftirqd/0/12
  CPU: 0 PID: 12 Comm: ksoftirqd/0 Not tainted 6.1.0 #1
  Call Trace:
    kasan_check_range (mm/kasan/generic.c:190)
    memcpy (mm/kasan/shadow.c:65)
    zlib_inflate (lib/zlib_inflate/inflate.c:458)
    z_decompress (drivers/net/ppp/ppp_deflate.c:460)
    ppp_receive_nonmp_frame (drivers/net/ppp/ppp_generic.c:2546 drivers/net/ppp/ppp_generic.c:2383)
    ppp_input (drivers/net/ppp/ppp_generic.c:2355 drivers/net/ppp/ppp_generic.c:2195 drivers/net/ppp/ppp_generic.c:2310)
    ppp_async_process (drivers/net/ppp/ppp_async.c:496)

  Allocated by task 12:
    __alloc_skb (net/core/skbuff.c:437 net/core/skbuff.c:509)
    __netdev_alloc_skb (net/core/skbuff.c:575)
    ppp_receive_nonmp_frame (include/linux/skbuff.h:3155 include/linux/skbuff.h:3168 drivers/net/ppp/ppp_generic.c:2539 drivers/net/ppp/ppp_generic.c:2383)
    ppp_input (drivers/net/ppp/ppp_generic.c:2355 drivers/net/ppp/ppp_generic.c:2195 drivers/net/ppp/ppp_generic.c:2310)
    ppp_async_process (drivers/net/ppp/ppp_async.c:496)

  The buggy address belongs to the object at ff110000021d1000
   which belongs to the cache kmalloc-512 of size 512
  The buggy address is located 68 bytes inside of
   512-byte region [ff110000021d1000, ff110000021d1200)

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Suggested-by: Paul Mackerras <paulus@ozlabs.org>
Reported-by: Kimi Security Team <bug-report@moonshot.ai>
Co-developed-by: Weiming Shi <shiweiming@moonshot.ai>
Signed-off-by: Weiming Shi <shiweiming@moonshot.ai>
Signed-off-by: Yilin Zhang <yilinzhang@moonshot.ai>
---
 drivers/net/ppp/ppp_generic.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Changes in v2:
- Set dev->min_mtu = 128 and reject MRU < 128 in PPPIOCSMRU per Paul's
  suggestion; drop the z_compress()/z_decompress() changes from v1.

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 1a610a1..4022117 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -72,6 +72,8 @@
 
 #define PPP_PROTO_LEN	2
 #define PPP_LCP_HDRLEN	4
+#define PPP_MIN_MTU	128
+#define PPP_MIN_MRU	128
 
 /* The filter instructions generated by libpcap are constructed
  * assuming a four-byte PPP header on each packet, where the last
@@ -808,6 +810,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case PPPIOCSMRU:
 		if (get_user(val, p))
 			break;
+		if (val < PPP_MIN_MRU || val > INT_MAX - PPP_HDRLEN - 1) {
+			err = -EINVAL;
+			break;
+		}
 		ppp_recv_lock(ppp);
 		ppp->mru = val;
 		ppp_recv_unlock(ppp);
@@ -1628,6 +1634,7 @@ static void ppp_setup(struct net_device *dev)
 
 	dev->hard_header_len = PPP_HDRLEN;
 	dev->mtu = PPP_MRU;
+	dev->min_mtu = PPP_MIN_MTU;
 	dev->addr_len = 0;
 	dev->tx_queue_len = 3;
 	dev->type = ARPHRD_PPP;
@@ -1719,10 +1726,11 @@ pad_compress_skb(struct ppp *ppp, struct sk_buff *skb)
 {
 	struct net_device *dev = netdev_from_priv(ppp);
 	struct sk_buff *new_skb;
+	int mtu = READ_ONCE(dev->mtu);
 	int len;
-	int new_skb_size = dev->mtu +
+	int new_skb_size = mtu +
 		ppp->xcomp->comp_extra + dev->hard_header_len;
-	int compressor_skb_size = dev->mtu +
+	int compressor_skb_size = mtu +
 		ppp->xcomp->comp_extra + PPP_HDRLEN;
 
 	if (skb_linearize(skb))
-- 
2.55.0

Re: [PATCH net v2] ppp: enforce minimum MTU/MRU to fix Deflate buffer underflows

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] The commit message asserts that setting dev->min_mtu = 128 and
  rejecting MRU < 128 "ensures that the output buffer seen by…
- [Low] The in-tree ABI description for PPPIOCSMRU was not updated for
  the new validation.

--

ppp: enforce minimum MTU/MRU to fix Deflate buffer underflows

The patch sets the PPP netdevice minimum MTU to 128, rejects PPPIOCSMRU
values below 128 (or large enough to overflow the allocation-size
calculation), and takes a single MTU snapshot in pad_compress_skb() so the
skb and compressor sizes cannot diverge.  The stated goal is that the
output buffer handed to z_compress() and z_decompress() always covers the
six-byte PPP/Deflate header so zlib's avail_out cannot wrap.

A couple of questions below.

> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 1a610a18893ba..402211768cf5f 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -72,6 +72,8 @@
>  
>  #define PPP_PROTO_LEN	2
>  #define PPP_LCP_HDRLEN	4
> +#define PPP_MIN_MTU	128
> +#define PPP_MIN_MRU	128
>  
>  /* The filter instructions generated by libpcap are constructed
>   * assuming a four-byte PPP header on each packet, where the last
> @@ -808,6 +810,10 @@ static long ppp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	case PPPIOCSMRU:
>  		if (get_user(val, p))
>  			break;
> +		if (val < PPP_MIN_MRU || val > INT_MAX - PPP_HDRLEN - 1) {
> +			err = -EINVAL;
> +			break;
> +		}

[Severity: Low]

Documentation/networking/ppp_generic.rst still describes PPPIOCSMRU as an
unconditional set:

  * PPPIOCSMRU sets the MRU (maximum receive unit) for the interface.
    The argument should point to an int containing the new MRU value.

No range and no failure mode are documented, while the ioctl now returns
-EINVAL for val < PPP_MIN_MRU or val > INT_MAX - PPP_HDRLEN - 1.

Should the documentation be updated with the accepted range and the new
error return?

Both bounds are also private defines in ppp_generic.c, so userspace that
includes include/uapi/linux/ppp-ioctl.h has no way to learn the minimum.
Would exporting PPP_MIN_MRU in the uapi header be preferable?

>  		ppp_recv_lock(ppp);
>  		ppp->mru = val;
>  		ppp_recv_unlock(ppp);
> @@ -1628,6 +1634,7 @@ static void ppp_setup(struct net_device *dev)
>  
>  	dev->hard_header_len = PPP_HDRLEN;
>  	dev->mtu = PPP_MRU;
> +	dev->min_mtu = PPP_MIN_MTU;
        ^^^^

[Severity: High]

The changelog says:

  "This ensures that the output buffer seen by z_compress() and
  z_decompress() always covers the PPP/Deflate header, so avail_out
  cannot wrap."

Does this hold on the transmit side?  z_compress() discards the
caller-supplied output size before subtracting the header:

drivers/net/ppp/ppp_deflate.c:z_compress() {
	/* Don't generate compressed packets which are larger than
	   the uncompressed packet. */
	if (osize > isize)
		osize = isize;
	...
	olen = PPP_HDRLEN + DEFLATE_OVHD;
	state->strm.next_out = wptr;
	state->strm.avail_out = oavail = osize - olen;
}

isize is pad_compress_skb()'s skb->len + 2, which does not depend on the
MTU.  ppp_write() only enforces a two-byte minimum:

drivers/net/ppp/ppp_generic.c:ppp_write() {
	if (count < PPP_PROTO_LEN)
		return -EINVAL;
}

and ppp_check_packet() applies the six-byte minimum only to PPP_LCP.  So a
two-byte write of 0x00 0x21 on a unit fd with Deflate running reaches
pad_compress_skb() with skb->len == 2, i.e. isize == 4:

ppp_write() -> ppp_xmit_process() -> __ppp_xmit_process() ->
ppp_prepare_tx_skb() -> pad_compress_skb() -> ppp->xcomp->compress()

z_compress() then clamps osize to 4 and avail_out (a uInt) becomes
4 - 6 == 0xfffffffe, which is the wrap the changelog says can no longer
occur, and this happens at the default MTU as well as at the new minimum.
zlib_deflate() only rejects avail_out == 0, so it proceeds with that bound.

I could not turn this into an out-of-bounds write after the patch: the wrap
is now only reachable when isize < 6, so avail_in is at most 3 and the few
bytes deflate emits still land inside the >= 132-byte skb, and s->pending
is drained by the discard iteration in z_compress() even with
next_out == NULL.  So the corruption looks gone, but the invariant the fix
is described as establishing does not exist.

Would it be better to keep the callee-side check that v1 had, i.e. have
z_compress() return 0 (or clamp) when osize < PPP_HDRLEN + DEFLATE_OVHD
after the isize clamp, or have pad_compress_skb() enforce a minimum frame
length?  Otherwise a compressor with a different comp_extra, an additional
->compress caller, or a later lowering of min_mtu silently restores the
unbounded avail_out.  Alternatively, could the changelog state that the
wrap is only rendered harmless on transmit, and why?

Note the receive side does look covered: z_decompress() uses the
caller-supplied osize unclamped and rejects isize <= PPP_HDRLEN +
DEFLATE_OVHD, so the new MRU floor does bound obuff_size there.

Separately, nothing here records the new dependency between
PPP_MIN_MTU/PPP_MIN_MRU and the six-byte PPP/Deflate header requirement
that lives in ppp_deflate.c.  Could a comment or a BUILD_BUG_ON be added
next to the new defines?

>  	dev->addr_len = 0;
>  	dev->tx_queue_len = 3;
>  	dev->type = ARPHRD_PPP;
> @@ -1719,10 +1726,11 @@ pad_compress_skb(struct ppp *ppp, struct sk_buff *skb)
>  {
>  	struct net_device *dev = netdev_from_priv(ppp);
>  	struct sk_buff *new_skb;
> +	int mtu = READ_ONCE(dev->mtu);
>  	int len;
> -	int new_skb_size = dev->mtu +
> +	int new_skb_size = mtu +
>  		ppp->xcomp->comp_extra + dev->hard_header_len;
> -	int compressor_skb_size = dev->mtu +
> +	int compressor_skb_size = mtu +
>  		ppp->xcomp->comp_extra + PPP_HDRLEN;
>  
>  	if (skb_linearize(skb))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908084116.634633-1-yilinzhang%40moonshot.ai