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

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ax25: fix OOB read after address header strip in ax25_rcv()
    
A crafted AX.25 frame with a valid address header but no control or PID
bytes causes skb->len to drop to zero after skb_pull() strips the
address header. The subsequent reads of skb->data[0] and skb->data[1]
are then out of bounds.

> diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
> index d75b3e9ed93de..d14ccebf9cdd5 100644
> --- a/net/ax25/ax25_in.c
> +++ b/net/ax25/ax25_in.c
> @@ -190,6 +190,9 @@ static int ax25_rcv(struct sk_buff *skb, struct net_device *dev,
>  	ax25_cb *ax25;
>  	ax25_dev *ax25_dev;
>  
> +	if (skb_linearize(skb))
> +		goto free;

Could calling skb_linearize() before verifying if the device is configured for
AX.25 introduce a regression?

Since ax25_packet_type is registered globally, ax25_rcv() receives all
ETH_P_AX25 packets from any network interface. skb_linearize() performs
memory allocations to coalesce fragmented packets. By performing this before
checking if ax25_dev is NULL, could an attacker send fragmented ETH_P_AX25
packets to an unconfigured interface to force unnecessary memory allocation
and CPU cycles?

Should the skb_linearize() call be moved after the device configuration check?

> +
>  	/*
>  	 *	Process the AX.25/LAPB frame.
>  	 */
[ ... ]
> @@ -217,6 +220,9 @@ static int ax25_rcv(struct sk_buff *skb, struct net_device *dev,
>  	 */
>  	skb_pull(skb, ax25_addr_size(&dp));
>  
> +	if (skb->len < 2)
> +		goto free;

Does this unconditionally drop all AX.25 LAPB control frames?

Standard AX.25 LAPB control frames (such as SABM, DISC, UA, DM, and RR)
consist solely of a 1-byte Control field and no PID byte. For these connection
management frames, skb->len is exactly 1 after the address header is pulled.
By discarding all frames shorter than 2 bytes, could this break the ability
to establish, acknowledge, or disconnect AX.25 connections?

Should the initial check only verify skb->len >= 1, and any requirement for
skb->len >= 2 be strictly scoped to the code paths that actually parse a PID
byte?

Also, if this skb->len < 2 check is relaxed, there is a pre-existing
out-of-bounds read in ax25_decode() for extended modulus frames.

If a malformed 1-byte frame is received on an extended modulus (AX25_EMODULUS)
connection, ax25_decode() unconditionally reads skb->data[1] for I-frames and
S-frames. It then calls skb_pull(skb, 2) but ignores the return value. Should
ax25_decode() validate that skb->len >= 2 before parsing the second control
byte for extended modulus frames?

Finally, there appears to be a pre-existing use-after-free and list corruption
in the ax25_rcv() error path when allocating memory for ax25->digipeat.

When handling an incoming connection request for a listener socket, the code
physically links the skb into the listener socket's receive queue via
skb_queue_head(&sk->sk_receive_queue, skb). Shortly after, it attempts to
allocate memory. If this allocation fails, the error path explicitly calls
kfree_skb(skb).

However, it fails to unlink the skb from sk->sk_receive_queue. (The subsequent
call to ax25_destroy_socket() flushes the newly created socket's queue, not
the listener socket's queue). This leaves a freed skb in the listener socket's
queue. When a user process later calls accept(), will it dequeue and
dereference the freed skb, resulting in a use-after-free?