* Re: [PATCH] net: wan: lapbether: validate frame length and headroom in lapbeth_rcv() and xmit
2026-09-19 22:34 [PATCH] net: wan: lapbether: validate frame length and headroom in lapbeth_rcv() and xmit Hui Peng
@ 2026-09-20 22:34 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-20 22:34 UTC (permalink / raw)
To: benquike
Cc: ms, andrew+netdev, davem, edumazet, kuba, pabeni, linux-x25,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 2 · Medium: 0 (1) · 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] The commit message describes two changes that are not present in
the diff.
- [High] In lapbeth_rcv() (drivers/net/wan/lapbether.c:135-142) the wire-
supplied 2-byte length `len = skb->data[0] + skb->data[1] * 256;` is…
Pre-existing issues:
- [Medium] lapbeth_rcv() increments `dev->stats.rx_packets/rx_bytes`
(drivers/net/wan/lapbether.c:141-142), where `dev` is the *Ethernet*…
--
net: wan: lapbether: validate frame length and headroom in lapbeth_rcv() and xmit
This changes lapbeth_rcv() to use pskb_trim_rcsum() instead of skb_trim()
when trimming the received frame to the length carried in the 2-byte
prefix, drops the frame if the trim fails, and moves the rx_packets and
rx_bytes accounting to after the trim.
A few questions below about the commit message and about the length
handling.
[Severity: High]
The commit message describes two changes that do not appear in the diff:
"call pskb_may_pull() before reading the 2-byte length prefix in
lapbeth_rcv() and ensure sufficient headroom in
lapbeth_data_transmit()"
The pskb_may_pull() call is already there before this patch, in
lapbeth_rcv():
if (!pskb_may_pull(skb, 2))
goto drop;
git blame attributes that to 1da177e4c3f4 ("Linux-2.6.12-rc2"), and the
diff adds no pskb_may_pull() call at all.
lapbeth_data_transmit() is not touched by the diff either, and the 2 bytes
it pushes are already accounted for in lapbeth_new_device():
/* When transmitting data:
* first this driver removes a pseudo header of 1 byte,
* then the lapb module prepends an LAPB header of at most 3 bytes,
* then this driver prepends a length field of 2 bytes,
* then the underlying Ethernet device prepends its own header.
*/
ndev->needed_headroom = -1 + 3 + 2 + dev->hard_header_len
+ dev->needed_headroom;
Could the changelog be rewritten to describe what the patch actually does,
namely the skb_trim() to pskb_trim_rcsum() conversion, the new drop path
and the relocated receive counters?
Relatedly, is the Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") tag correct
here? As written it points at an unchecked length-prefix read that the
patch does not change, which may lead to this being picked up for stable
as a memory-safety fix.
> diff --git a/drivers/net/wan/lapbether.c b/drivers/net/wan/lapbether.c
> index c3630a82913b..3838910ea7bd 100644
> --- a/drivers/net/wan/lapbether.c
> +++ b/drivers/net/wan/lapbether.c
> @@ -133,11 +133,13 @@ static int lapbeth_rcv(struct sk_buff *skb, struct net_device *dev,
> goto drop_unlock;
>
> len = skb->data[0] + skb->data[1] * 256;
> - dev->stats.rx_packets++;
> - dev->stats.rx_bytes += len;
>
> skb_pull(skb, 2); /* Remove the length bytes */
> - skb_trim(skb, len); /* Set the length of the data */
> + if (pskb_trim_rcsum(skb, len))
> + goto drop_unlock;
[Severity: High]
Does this actually validate len against what was received?
len comes straight off the wire, and pskb_trim_rcsum() returns success
without looking at the skb whenever the requested length is not smaller
than the current one:
include/linux/skbuff.h:pskb_trim_rcsum() {
skb_might_realloc(skb);
if (likely(len >= skb->len))
return 0;
return pskb_trim_rcsum_slow(skb, len);
}
So for a frame whose prefix declares more bytes than were actually
received, the new check returns 0, no trim happens, and the untrimmed skb
(including any Ethernet minimum-frame padding) is handed to
lapb_data_received(). Only the shrink path can fail, via
pskb_trim_rcsum_slow() returning -EINVAL for CHECKSUM_PARTIAL or -ENOMEM
from __pskb_trim.
The transmit side in the same file defines the prefix as the real payload
length:
int size = skb->len;
ptr = skb_push(skb, 2);
*ptr++ = size % 256;
*ptr++ = size / 256;
Would an explicit check be closer to what the subject line promises?
if (len > skb->len)
goto drop_unlock;
> +
> + dev->stats.rx_packets++;
> + dev->stats.rx_bytes += len;
[Severity: High]
Can a sender on the same Ethernet segment inflate rx_bytes here? len is
the declared length, not the number of bytes received, and per the above
it is never rejected when it is too large.
A ~60-byte frame declaring 65535 adds 65535 to dev->stats.rx_bytes, which
does not match the rtnl_link_stats64 definition of rx_bytes as good
received bytes corresponding to rx_packets. Would counting skb->len after
the trim be more accurate?
[Severity: Medium]
This isn't a bug introduced by this patch (the device selection dates back
to ea2ebaf822ad7), but since these two lines are being moved anyway: is dev
the intended netdev here? dev is the Ethernet device the packet_type
handler was called on, while the transmit counterpart charges the LAPB
device:
lapbeth_data_transmit() {
ndev->stats.tx_packets++;
ndev->stats.tx_bytes += size;
}
The result is that ip -s link show lapb0 reports transmit counters but
never any rx_packets/rx_bytes, since the receive path reaches the axdev
only through lapb_data_received() -> lapbeth_data_indication() ->
netif_receive_skb_core(), which does not touch dev->stats.
There is also the question of who owns that counter block. dev->stats is a
plain non-atomic struct net_device_stats belonging to the Ethernet driver,
and this read-modify-write is serialized only by lapbeth->up_lock, which
that driver does not take:
spin_lock_bh(&lapbeth->up_lock);
...
dev->stats.rx_packets++;
dev->stats.rx_bytes += len;
Can this lose increments against the Ethernet driver's own dev->stats
updates, or produce torn rx_bytes on 32-bit? Would lapbeth->axdev->stats be
the right target?
>
> err = lapb_data_received(lapbeth->axdev, skb);
> if (err != LAPB_OK) {
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919223433.3882618-1-benquike%40gmail.com
^ permalink raw reply [flat|nested] 2+ messages in thread