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