* Re: [PATCH] ppp: fix bsd_decompress() OOB read and ppp_decompress_frame() headroom check
2026-09-19 21:52 [PATCH] ppp: fix bsd_decompress() OOB read and ppp_decompress_frame() headroom check Hui Peng
@ 2026-09-20 22:54 ` netdev-bot+sashiko
2026-09-22 14:25 ` Simon Horman
2026-09-21 15:10 ` krzk
1 sibling, 1 reply; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-09-20 22:54 UTC (permalink / raw)
To: benquike
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, linux-ppp, netdev,
linux-kernel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 3 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] ppp_decompress_frame() (drivers/net/ppp/ppp_generic.c:2555-2557)
now calls pskb_expand_head(skb, 2, 0, GFP_ATOMIC) unconditionally on…
- [Medium] The commit message states that the decompressed length 'is
only checked for len < 0 before indexing ns->data[2..3] and calling…
- [Medium] The patch hardens only one of the three in-tree implementers
of `struct compressor::decompress`.
- [Medium] The new pskb_expand_head(skb, 2, 0, GFP_ATOMIC) in
ppp_decompress_frame() (drivers/net/ppp/ppp_generic.c:2556) creates…
--
ppp: fix bsd_decompress() OOB read and ppp_decompress_frame() headroom check
This adds an isize floor check to bsd_decompress(), makes
ppp_decompress_frame() call pskb_expand_head(skb, 2, 0, GFP_ATOMIC) to
guarantee two bytes of headroom, and tightens the decompressed length
check from len < 0 to len < PPP_HDRLEN.
No Link: tags were present in the commit header.
A few questions below.
> ppp: fix bsd_decompress() OOB read and ppp_decompress_frame() headroom check
>
> In ppp_decompress_frame(), skb->data - 2 is passed to the decompressor
> assuming at least 2 bytes of headroom exist for the PPP Address/Control
> fields, and the returned decompressed length is only checked for len < 0
> before indexing ns->data[2..3] and calling skb_pull_rcsum(skb, 2).
[Severity: Medium]
Does this describe the code being patched? Neither an ns->data[2..3]
access nor a skb_pull_rcsum(skb, 2) exists in ppp_decompress_frame().
The post-decompress sequence there is:
consume_skb(skb);
skb = ns;
skb_put(skb, len);
skb_pull(skb, 2); /* pull off the A/C bytes */
/* Don't call __ppp_decompress_proto() here, but instead rely on
* corresponding algo (mppe/bsd/deflate) to decompress it.
*/
The only skb_pull_rcsum(skb, 2) in the file lives in the caller,
ppp_receive_nonmp_frame(). The read that the new len < PPP_HDRLEN check
actually protects looks like the two-byte
proto = PPP_PROTO(skb);
in ppp_receive_nonmp_frame() after skb->len has been reduced to 1 by
skb_put(3) + skb_pull(2), which the message does not mention. Could the
message be updated to match the tree being patched, so that stable
backport decisions can be checked against it?
> diff --git a/drivers/net/ppp/bsd_comp.c b/drivers/net/ppp/bsd_comp.c
> index 63a6d251c746..03ee4ff4ec1b 100644
> --- a/drivers/net/ppp/bsd_comp.c
> +++ b/drivers/net/ppp/bsd_comp.c
> @@ -847,6 +847,8 @@ static int bsd_decompress (void *state, unsigned char *ibuf, int isize,
> int extra;
>
> db = (struct bsd_db *) state;
> + if (isize < PPP_HDRLEN + BSD_OVHD)
> + return DECOMP_ERROR;
> max_ent = db->max_ent;
[Severity: Medium]
Should this check instead live in the shared caller? There are three
in-tree implementers of struct compressor::decompress, and they are all
reached from the single dispatch point in ppp_decompress_frame():
len = ppp->rcomp->decompress(ppp->rc_state, skb->data - 2,
skb->len + 2, ns->data, obuff_size);
z_decompress() already validates isize before touching ibuf, and this
patch adds the same for bsd_decompress(), but mppe_decompress() still
reads ibuf[4] in a variable initializer that runs before its own length
guard:
drivers/net/ppp/ppp_mppe.c:mppe_decompress() {
int flushed = MPPE_BITS(ibuf) & MPPE_BIT_FLUSHED;
if (isize <= PPP_HDRLEN + MPPE_OVHD) {
...
return DECOMP_ERROR;
}
with
#define MPPE_BITS(p) ((p)[4] & 0xf0)
The receive path only guarantees skb->len >= 2 (ppp_decompress_proto()
does pskb_may_pull(skb, 2)), so a peer that has negotiated MPPE and sends
a 2-byte PPP_COMP frame gives isize == 4 and makes ibuf[4] read one byte
past the frame data. Does a single minimum-isize test at the dispatch
point cover all three callbacks rather than leaving MPPE inconsistent?
Also, the indentation of the added lines uses tabs while the surrounding
function uses the file's older 4-space style.
> diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
> index 1a610a18893b..f7af328b7bf8 100644
> --- a/drivers/net/ppp/ppp_generic.c
> +++ b/drivers/net/ppp/ppp_generic.c
> @@ -2552,7 +2552,8 @@ ppp_decompress_frame(struct ppp *ppp, struct sk_buff *skb)
> /* Until we fix all the decompressor's need to make sure
> * data portion is linear.
> */
> - if (!pskb_may_pull(skb, skb->len))
> + if (!pskb_may_pull(skb, skb->len) ||
> + pskb_expand_head(skb, 2, 0, GFP_ATOMIC))
> goto err;
[Severity: High]
Is the expansion meant to be unconditional? pskb_expand_head() has no
"headroom is already sufficient" fast path:
net/core/skbuff.c:pskb_expand_head() {
data = kmalloc_reserve(&size, gfp_mask, NUMA_NO_NODE, NULL);
...
memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head);
...
skb_free_head(skb);
...
skb->truesize += size - osize;
}
nhead is additional headroom, so every received frame now takes a
kmalloc + memcpy + free even though in-tree channels already reserve
headroom, for example:
drivers/net/ppp/ppp_async.c:ppp_async_input() {
if (buf[0] != PPP_ALLSTATIONS)
skb_reserve(skb, 2 + (buf[0] & 1));
That skb_reserve() also arranges 4-byte payload alignment, which the
unconditional reallocation discards. The call also sits before the
proto == PPP_COMP test, so it runs for frames that never dereference
skb->data - 2 at all (the else branch only calls ->incomp() when
ppp->rcomp->incomp is non-NULL).
Separately, does a transient GFP_ATOMIC failure here have larger
consequences than dropping one frame? The failure funnels into:
err:
ppp->rstate |= SC_DC_ERROR;
ppp_receive_error(ppp);
and the caller only enters the decompressor while that bit is clear:
drivers/net/ppp/ppp_generic.c:ppp_receive_nonmp_frame() {
if (ppp->rc_state && (ppp->rstate & SC_DECOMP_RUN) &&
(ppp->rstate & (SC_DC_FERROR | SC_DC_ERROR)) == 0)
skb = ppp_decompress_frame(ppp, skb);
SC_DC_ERROR is only cleared from ppp_ccp_peek() via
ppp->rstate &= ~SC_DC_ERROR;
so one momentary atomic allocation failure appears to disable receive
decompression for the whole link until a CCP Reset round trip. Would
skb_cow_head(skb, 2), or an explicit
if (skb_headroom(skb) < 2 && pskb_expand_head(skb, 2, 0, GFP_ATOMIC))
placed inside the branch that actually uses skb->data - 2, avoid both the
per-packet copy and the new sticky failure mode?
[Severity: Medium]
Are the two new headroom bytes ever initialised? pskb_expand_head()
allocates with kmalloc_reserve() (no __GFP_ZERO) and copies the old head
to data + nhead, so data[0] and data[1] are never written. Afterwards
skb->data == data + 2 + old_headroom, so when the incoming skb had less
than 2 bytes of headroom, skb->data - 2 points into that uninitialised
prefix, and that is exactly the case this change exists to handle.
Those bytes are then handed to the decompressor as the PPP
Address/Control octets:
/* the decompressor still expects the A/C bytes in the hdr */
len = ppp->rcomp->decompress(ppp->rc_state, skb->data - 2,
skb->len + 2, ns->data, obuff_size);
and in the uncompressed branch to:
if (ppp->rcomp->incomp)
ppp->rcomp->incomp(ppp->rc_state, skb->data - 2,
skb->len + 2);
bsd_decompress() consumes them and copies them into the output header:
adrs = PPP_ADDRESS (ibuf);
ctrl = PPP_CONTROL (ibuf);
z_decompress() and mppe_decompress() do the same. Headroom below 2 is
reachable on the tty path: ppp_async_input() skips skb_reserve() when the
frame starts with PPP_ALLSTATIONS, process_input_packet() pulls the two
A/C bytes leaving headroom 2, and then a PFC-compressed protocol field
consumes one more byte in __ppp_decompress_proto():
if (ppp_skb_is_compressed_proto(skb))
*(u8 *)skb_push(skb, 1) = 0x00;
Should the two bytes be filled with PPP_ALLSTATIONS/PPP_UI after the
expansion, so the decompressors read the header the comment says they
expect?
> @@ -2576,7 +2577,7 @@ ppp_decompress_frame(struct ppp *ppp, struct sk_buff *skb)
> /* the decompressor still expects the A/C bytes in the hdr */
> len = ppp->rcomp->decompress(ppp->rc_state, skb->data - 2,
> skb->len + 2, ns->data, obuff_size);
> - if (len < 0) {
> + if (len < PPP_HDRLEN) {
> /* Pass the compressed frame to pppd as an
> error indication. */
> if (len == DECOMP_FATALERROR)
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919215233.3470411-1-benquike%40gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] ppp: fix bsd_decompress() OOB read and ppp_decompress_frame() headroom check
2026-09-19 21:52 [PATCH] ppp: fix bsd_decompress() OOB read and ppp_decompress_frame() headroom check Hui Peng
2026-09-20 22:54 ` netdev-bot+sashiko
@ 2026-09-21 15:10 ` krzk
1 sibling, 0 replies; 4+ messages in thread
From: krzk @ 2026-09-21 15:10 UTC (permalink / raw)
To: Hui Peng
Cc: linux-kernel, linux-ppp, edumazet, kuba, netdev, pabeni, davem,
andrew+netdev
On Sat, 19 Sep 2026 21:52:33 +0000, Hui Peng wrote:
> In ppp_decompress_frame(), skb->data - 2 is passed to the decompressor
> assuming at least 2 bytes of headroom exist for the PPP Address/Control
> fields, and the returned decompressed length is only checked for len < 0
> before indexing ns->data[2..3] and calling skb_pull_rcsum(skb, 2).
>
> Ensure 2 bytes of headroom via pskb_expand_head(skb, 2, 0, GFP_ATOMIC),
> reject decompressed frames shorter than PPP_HDRLEN (4 bytes), and check
> isize >= PPP_HDRLEN + BSD_OVHD in bsd_decompress() before reading the
> sequence number and header bytes.
>
> Fixes: 224cf5ad14c0 ("ppp: Move the PPP drivers")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
You sent multiple independent patches, to multiple independent
subsystems. The amount of these patches clearly suggest this was
AI generated and most likely not tested.
More importantly, you sent all this work without properly organizing
relevant patches into patchsets. This makes reviewing difficult
and might cause multiple reviewers to address the same issue.
Replying to the entire set is impossible and requires handling each
patch independently, instead of applying or discarding the set.
Maintainers also won't see the bigger picture of your work. Quite
worrying.
This is on the verge of hostile patch: bomb us with so many
contributions, we won't be able to handle them in efficient manner,
like responding ONCE to ask you to slow down. Considering all this
is untested and LLM generated, I have even more doubts whether this
should be considered for review.
Please read kernel documentation BEFORE posting more work. It will
explain you how to identify subsystems, how to organize your work per
subsystem, how to document usage of LLM and how what you should not
do if this was posted in a good faith.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 4+ messages in thread