net/lapb/lapb_subr.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
`lapb_decode()` populates a caller-allocated `struct lapb_frame frame`
on the stack of `lapb_data_input()`, but does not zero-initialize
`*frame`.
1. When `skb->data[0]` is not a valid LAPB address byte (`LAPB_ADDR_A`,
`LAPB_ADDR_B`, `LAPB_ADDR_C`, or `LAPB_ADDR_D`), `frame->cr` is left
uninitialized on the stack while `lapb_decode()` continues parsing
and copies `frame->cr` into `lapb->frmr_data.cr` via
`lapb_transmit_frmr()`, leaking 1 bit of kernel stack memory over the
network.
2. When decoding an unnumbered (`LAPB_U`) or illegal frame in
`LAPB_EXTENDED` mode, `frame->control[1]` is left uninitialized on
the stack and copied into the outgoing `FRMR` frame by
`lapb_transmit_frmr()`.
Zero-initialize `*frame` at the start of `lapb_decode()` and return `-1`
when `skb->data[0]` is not a valid LAPB address byte.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
net/lapb/lapb_subr.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/net/lapb/lapb_subr.c b/net/lapb/lapb_subr.c
index 592a22d86a97..8150f3583f15 100644
--- a/net/lapb/lapb_subr.c
+++ b/net/lapb/lapb_subr.c
@@ -106,6 +106,7 @@ int lapb_validate_nr(struct lapb_cb *lapb, unsigned short nr)
int lapb_decode(struct lapb_cb *lapb, struct sk_buff *skb,
struct lapb_frame *frame)
{
+ memset(frame, 0, sizeof(*frame));
frame->type = LAPB_ILLEGAL;
lapb_dbg(2, "(%p) S%d RX %3ph\n", lapb->dev, lapb->state, skb->data);
@@ -120,25 +121,33 @@ int lapb_decode(struct lapb_cb *lapb, struct sk_buff *skb,
if (lapb->mode & LAPB_DCE) {
if (skb->data[0] == LAPB_ADDR_D)
frame->cr = LAPB_COMMAND;
- if (skb->data[0] == LAPB_ADDR_C)
+ else if (skb->data[0] == LAPB_ADDR_C)
frame->cr = LAPB_RESPONSE;
+ else
+ return -1;
} else {
if (skb->data[0] == LAPB_ADDR_C)
frame->cr = LAPB_COMMAND;
- if (skb->data[0] == LAPB_ADDR_D)
+ else if (skb->data[0] == LAPB_ADDR_D)
frame->cr = LAPB_RESPONSE;
+ else
+ return -1;
}
} else {
if (lapb->mode & LAPB_DCE) {
if (skb->data[0] == LAPB_ADDR_B)
frame->cr = LAPB_COMMAND;
- if (skb->data[0] == LAPB_ADDR_A)
+ else if (skb->data[0] == LAPB_ADDR_A)
frame->cr = LAPB_RESPONSE;
+ else
+ return -1;
} else {
if (skb->data[0] == LAPB_ADDR_A)
frame->cr = LAPB_COMMAND;
- if (skb->data[0] == LAPB_ADDR_B)
+ else if (skb->data[0] == LAPB_ADDR_B)
frame->cr = LAPB_RESPONSE;
+ else
+ return -1;
}
}
--
2.55.0.1082.g2b9226bbc0-googThank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 2 (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).
- [Medium] The second justification in the commit message is factually
wrong.
- [Medium] The patch bundles an orthogonal receive-path acceptance-policy
change into what is presented as an uninitialized-memory fix.
Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but there is a
producer/consumer mismatch in net/lapb around LAPB_ILLEGAL.
--
net: lapb: fix uninitialized stack frame fields in lapb_decode()
lapb_decode() fills in a caller-allocated struct lapb_frame that lives on
the stack of lapb_data_input(). The patch adds a memset() of that struct at
the top of lapb_decode() and converts the address-byte comparisons into an
if/else-if chain that returns -1 when the address byte matches none of the
expected values.
No Link: tags in the commit header.
A few questions about the commit message and the behavioural change below.
> 1. When `skb->data[0]` is not a valid LAPB address byte (`LAPB_ADDR_A`,
> `LAPB_ADDR_B`, `LAPB_ADDR_C`, or `LAPB_ADDR_D`), `frame->cr` is left
> uninitialized on the stack while `lapb_decode()` continues parsing
> and copies `frame->cr` into `lapb->frmr_data.cr` via
> `lapb_transmit_frmr()`, leaking 1 bit of kernel stack memory over the
> network.
> 2. When decoding an unnumbered (`LAPB_U`) or illegal frame in
> `LAPB_EXTENDED` mode, `frame->control[1]` is left uninitialized on
> the stack and copied into the outgoing `FRMR` frame by
> `lapb_transmit_frmr()`.
[Severity: Medium]
Is justification 2 accurate? In the LAPB_EXTENDED branch of lapb_decode()
the U-frame arm already assigns control[1] explicitly, and that line is not
touched by this patch:
} else if ((skb->data[0] & LAPB_U) == 3) {
/*
* U frame - take out PF
*/
frame->type = skb->data[0] & ~LAPB_SPF;
frame->pf = skb->data[0] & LAPB_SPF;
frame->control[0] = skb->data[0];
frame->control[1] = 0x00;
The I and S arms in the same branch both assign control[1] = skb->data[1].
For the "illegal frame in LAPB_EXTENDED mode" half of the claim: with
LAPB_S == 0x01 and LAPB_U == 0x03, the three tests
if (!(skb->data[0] & LAPB_S))
else if ((skb->data[0] & LAPB_U) == 1)
else if ((skb->data[0] & LAPB_U) == 3)
cover all 256 control-byte values, since bit 0 clear selects the I arm and
bit 0 set forces skb->data[0] & 3 to be 1 or 3. Is there any extended-mode
path that returns 0 without writing control[1]?
The only branch that leaves control[1] unwritten is the standard
(non-extended) branch, which the commit message does not mention, and
lapb_transmit_frmr() emits control[1] only in its LAPB_EXTENDED arm:
if (lapb->mode & LAPB_EXTENDED) {
...
*dptr++ = lapb->frmr_data.control[1];
} else {
dptr = skb_put(skb, 4);
*dptr++ = LAPB_FRMR;
*dptr++ = lapb->frmr_data.control[0];
Since lapb->mode does not change between decode and FRMR transmission, can
the standard-mode control[1] byte reach the wire either?
Justification 1 does look real: frame->cr was left uninitialized when the
address byte matched neither expected value, and it is encoded into the
transmitted FRMR by
if (lapb->frmr_data.cr == LAPB_RESPONSE)
*dptr |= 0x01;
Could the message be narrowed to that one leak? The Fixes: 1da177e4c3f4
tag means this goes to every stable tree, and the current text makes the
issue look broader than it is.
> diff --git a/net/lapb/lapb_subr.c b/net/lapb/lapb_subr.c
> index 592a22d86a97..8150f3583f15 100644
> --- a/net/lapb/lapb_subr.c
> +++ b/net/lapb/lapb_subr.c
> @@ -106,6 +106,7 @@ int lapb_validate_nr(struct lapb_cb *lapb, unsigned short nr)
> int lapb_decode(struct lapb_cb *lapb, struct sk_buff *skb,
> struct lapb_frame *frame)
> {
> + memset(frame, 0, sizeof(*frame));
> frame->type = LAPB_ILLEGAL;
[Severity: Medium]
This isn't a bug introduced by this patch, but the memset() sits right on
top of a producer/consumer mismatch worth noting. lapb_decode() advertises
an illegal-frame result via frame->type = LAPB_ILLEGAL, yet no successful
return can leave that value in place: LAPB_ILLEGAL is 0x100, while the
classification arms assign LAPB_I (0), skb->data[0] & 0x0F (0-15) or
skb->data[0] & ~LAPB_SPF (0-255), and as noted above the arms are exhaustive
over the control byte.
The consumer side in lapb_state3_machine() therefore never runs:
case LAPB_ILLEGAL:
lapb_dbg(1, "(%p) S3 RX ILLEGAL(%d)\n", lapb->dev, frame->pf);
lapb->frmr_data = *frame;
lapb->frmr_type = LAPB_FRMR_W;
lapb_transmit_frmr(lapb);
There is no default: arm in that switch, so an extended-mode S frame with
skb->data[0] & 0x0F == 0x0D matches no case and is simply dropped at
if (!queued) kfree_skb(skb), rather than producing the FRMR(W) response.
One note specific to the new line: LAPB_I is 0x00, so the memset() must
always be followed by the explicit frame->type = LAPB_ILLEGAL assignment.
If that assignment were ever dropped or reordered, an undecodable frame
would reach the state machines looking like a valid I frame. Would a
comment there help?
> @@ -120,25 +121,33 @@ int lapb_decode(struct lapb_cb *lapb, struct sk_buff *skb,
> if (lapb->mode & LAPB_DCE) {
> if (skb->data[0] == LAPB_ADDR_D)
> frame->cr = LAPB_COMMAND;
> - if (skb->data[0] == LAPB_ADDR_C)
> + else if (skb->data[0] == LAPB_ADDR_C)
> frame->cr = LAPB_RESPONSE;
> + else
> + return -1;
[Severity: Medium]
Should the new early returns be a separate patch? They change what the
receive path accepts, which is independent of the uninitialized-memory fix.
Before this patch an unexpected address byte only failed to set frame->cr;
decoding continued through skb_pull(skb, 1) into the control-field parser
and the frame still reached the state machine, so a SABM/UA/DISC carrying a
non-conforming address could still drive connection setup and teardown.
After this patch the sole caller drops the frame outright:
net/lapb/lapb_in.c:lapb_data_input() {
if (lapb_decode(lapb, skb, &frame) < 0) {
kfree_skb(skb);
return;
}
No state machine is entered, no UA/DM/FRMR reply is generated, nothing is
logged, and the trailing lapb_kick(lapb) is skipped. On a link whose peer
address byte does not match the local MLP/SLP plus DCE/DTE configuration,
the link can no longer leave LAPB_STATE_0, and there is no counter or
message to diagnose it.
Is the early return needed for the leak itself? With the memset() in place
frame->cr is 0, which is neither LAPB_COMMAND (1) nor LAPB_RESPONSE (2), and
those are the only values ever tested, in lapb_transmit_frmr():
if (lapb->frmr_data.cr == LAPB_RESPONSE)
*dptr |= 0x10;
and in lapb_check_need_response():
if (type == LAPB_COMMAND && pf)
lapb_enquiry_response(lapb);
Dropping invalid-address frames does match ISO 7776, so the change looks
desirable on its own, but the commit message describes only the mechanical
change ("return -1 when skb->data[0] is not a valid LAPB address byte") and
not the protocol consequence. Could this be split into zero-initializing
the frame and rejecting invalid address bytes, or at least have the
behavioural effect spelled out for backporters?
> } else {
> if (skb->data[0] == LAPB_ADDR_C)
> frame->cr = LAPB_COMMAND;
> - if (skb->data[0] == LAPB_ADDR_D)
> + else if (skb->data[0] == LAPB_ADDR_D)
> frame->cr = LAPB_RESPONSE;
> + else
> + return -1;
> }
[ ... remaining address-byte arms are the same change ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919213639.3314894-1-benquike%40gmail.com
© 2016 - 2026 Red Hat, Inc.