[v1] ax25: fix OOB read after address header strip in ax25_rcv().

[PATCH] ax25: fix OOB read after address header strip in ax25_rcv().

Posted by Ashutosh Desai 2 months, 1 week ago

ax25_rcv() calls skb_pull(skb, ax25_addr_size(&dp)) to strip the
address header, then immediately reads skb->data[0] and skb->data[1]
without verifying the buffer still contains at least 2 bytes.

A crafted 15-byte KISS frame (1 KISS byte + 14 address bytes with
EBIT set in the source address, no control/PID bytes) passes
ax25_addr_parse() which only requires len >= 14, and passes the KISS
byte check (low nibble == 0). After skb_pull(1) in ax25_kiss_rcv()
and skb_pull(14) in ax25_rcv(), skb->len is 0 and the subsequent
reads of skb->data[0] (control byte) and skb->data[1] (PID byte)
are out of bounds.

Add a check that at least 2 bytes remain after stripping the address
header, freeing the skb and returning on malformed input.

Signed-off-by: Ashutosh Desai <ashutoshdesai993@gmail.com>
---
 net/ax25/ax25_in.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
index d75b3e9ed..92baac77f 100644
--- a/net/ax25/ax25_in.c
+++ b/net/ax25/ax25_in.c
@@ -217,6 +217,11 @@ static int ax25_rcv(struct sk_buff *skb, struct net_device *dev,
 	 */
 	skb_pull(skb, ax25_addr_size(&dp));
 
+	if (skb->len < 2) {
+		kfree_skb(skb);
+		return 0;
+	}
+
 	/* For our port addresses ? */
 	if (ax25cmp(&dest, dev_addr) == 0 && dp.lastrepeat + 1 == dp.ndigi)
 		mine = 1;
-- 
2.34.1

Re: [PATCH] ax25: fix OOB read after address header strip in ax25_rcv().

Posted by Eric Dumazet 2 months ago

On Wed, Apr 8, 2026 at 6:22 PM Ashutosh Desai
<ashutoshdesai993@gmail.com> wrote:
>
> ax25_rcv() calls skb_pull(skb, ax25_addr_size(&dp)) to strip the
> address header, then immediately reads skb->data[0] and skb->data[1]
> without verifying the buffer still contains at least 2 bytes.
>
> A crafted 15-byte KISS frame (1 KISS byte + 14 address bytes with
> EBIT set in the source address, no control/PID bytes) passes
> ax25_addr_parse() which only requires len >= 14, and passes the KISS
> byte check (low nibble == 0). After skb_pull(1) in ax25_kiss_rcv()
> and skb_pull(14) in ax25_rcv(), skb->len is 0 and the subsequent
> reads of skb->data[0] (control byte) and skb->data[1] (PID byte)
> are out of bounds.
>
> Add a check that at least 2 bytes remain after stripping the address
> header, freeing the skb and returning on malformed input.
>
> Signed-off-by: Ashutosh Desai <ashutoshdesai993@gmail.com>
> ---
>  net/ax25/ax25_in.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
> index d75b3e9ed..92baac77f 100644
> --- a/net/ax25/ax25_in.c
> +++ b/net/ax25/ax25_in.c
> @@ -217,6 +217,11 @@ static int ax25_rcv(struct sk_buff *skb, struct net_device *dev,
>          */
>         skb_pull(skb, ax25_addr_size(&dp));
>
> +       if (skb->len < 2) {
> +               kfree_skb(skb);
> +               return 0;
> +       }
> +

Are you aware of pskb_may_pull() ?

Testing skb->len is not enough.

I suspect all net/ax25 is expecting linear skbs and never considered fragments.

[PATCH v2] ax25: fix OOB read after address header strip in ax25_rcv().

Posted by Ashutosh Desai 2 months ago

ax25_rcv() calls skb_pull(skb, ax25_addr_size(&dp)) to strip the
address header, then reads skb->data[0] (control byte) and skb->data[1]
(PID byte) without verifying those bytes are in the linear sk_buff area.

The original fix checked skb->len < 2, but as Eric Dumazet pointed out,
skb->len counts bytes across the linear head and any non-linear
fragments. If the two bytes needed sit in a fragment, the check passes
but the direct skb->data access is still out of bounds.

Use pskb_may_pull(skb, 2) instead, which ensures both bytes are present
and pulls them into the linear area if needed before we read them.

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Ashutosh Desai <ashutoshdesai993@gmail.com>
---
 net/ax25/ax25_in.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
index d75b3e9ed..6a71dea87 100644
--- a/net/ax25/ax25_in.c
+++ b/net/ax25/ax25_in.c
@@ -217,6 +217,11 @@ static int ax25_rcv(struct sk_buff *skb, struct net_device *dev,
 	 */
 	skb_pull(skb, ax25_addr_size(&dp));
 
+	if (!pskb_may_pull(skb, 2)) {
+		kfree_skb(skb);
+		return 0;
+	}
+
 	/* For our port addresses ? */
 	if (ax25cmp(&dest, dev_addr) == 0 && dp.lastrepeat + 1 == dp.ndigi)
 		mine = 1;
-- 
2.34.1

Re: [PATCH v2] ax25: fix OOB read after address header strip in ax25_rcv().

Posted by Eric Dumazet 2 months ago

On Thu, Apr 9, 2026 at 8:24 AM Ashutosh Desai
<ashutoshdesai993@gmail.com> wrote:
>
> ax25_rcv() calls skb_pull(skb, ax25_addr_size(&dp)) to strip the
> address header, then reads skb->data[0] (control byte) and skb->data[1]
> (PID byte) without verifying those bytes are in the linear sk_buff area.
>
> The original fix checked skb->len < 2, but as Eric Dumazet pointed out,
> skb->len counts bytes across the linear head and any non-linear
> fragments. If the two bytes needed sit in a fragment, the check passes
> but the direct skb->data access is still out of bounds.
>
> Use pskb_may_pull(skb, 2) instead, which ensures both bytes are present
> and pulls them into the linear area if needed before we read them.
>
> Suggested-by: Eric Dumazet <edumazet@google.com>

I have not suggested to fix a bug in ax25.
I added a comment on your initial version.
A "Suggested-by" would imply I made the initial discovery.

Please carefully read Documentation/process/maintainer-netdev.rst

We require a 24-hour delay between each version.

[PATCH v2] rose: fix OOB reads on short CLEAR REQUEST frames.

Posted by Ashutosh Desai 2 months ago

rose_process_rx_frame() passes skb directly to rose_decode(), which
reads skb->data[2] without any prior length check. For CLEAR REQUEST
frames the state machines then read skb->data[3] and skb->data[4] as
the cause and diagnostic bytes.

The original fix checked skb->len after the rose_decode() call, which
was wrong for two reasons: rose_decode() already accessed skb->data[2]
before the check ran, and skb->len counts bytes across non-linear
fragments while skb->data only covers the linear head - so even a
passing len check doesn't guarantee the bytes are safe to read directly.

Eric Dumazet pointed out that pskb_may_pull() is the right approach
here. Add a pskb_may_pull(skb, 3) check before rose_decode() to cover
its frame[2] access, and a pskb_may_pull(skb, 5) check afterwards for
the CLEAR REQUEST path to cover the cause and diagnostic reads.

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Ashutosh Desai <ashutoshdesai993@gmail.com>
---
 net/rose/rose_in.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/rose/rose_in.c b/net/rose/rose_in.c
index 0276b393f..b9f01a11e 100644
--- a/net/rose/rose_in.c
+++ b/net/rose/rose_in.c
@@ -269,8 +269,18 @@ int rose_process_rx_frame(struct sock *sk, struct sk_buff *skb)
 	if (rose->state == ROSE_STATE_0)
 		return 0;
 
+	if (!pskb_may_pull(skb, 3)) {
+		kfree_skb(skb);
+		return 0;
+	}
+
 	frametype = rose_decode(skb, &ns, &nr, &q, &d, &m);
 
+	if (frametype == ROSE_CLEAR_REQUEST && !pskb_may_pull(skb, 5)) {
+		kfree_skb(skb);
+		return 0;
+	}
+
 	switch (rose->state) {
 	case ROSE_STATE_1:
 		queued = rose_state1_machine(sk, skb, frametype);
-- 
2.34.1