Re: [PATCH net 1/2] net: hamradio: bpqether: validate frame length in bpq_rcv()

[Joerg Reuter]

Am Thu, Apr 09, 2026 at 01:23:57AM +0800 schrieb Mashiro Chen:
> The BPQ length field is decoded as:
> 
>   len = skb->data[0] + skb->data[1] * 256 - 5;
> 
> If the sender sets bytes [0..1] to values whose combined value is
> less than 5, len becomes negative.  Passing a negative int to
> skb_trim() silently converts to a huge unsigned value, causing the
> function to be a no-op.  The frame is then passed up to AX.25 with
> its original (untrimmed) payload, delivering garbage beyond the
> declared frame boundary.

I don't even know why there is a length field in the first place, and John
G8BPQ doesn't seem to remember either. 

There is nothing supposed to come after the payload, and there should be no
need to skb_trim() at all. 

However, since an obviously wrong length field indicates that something is
indeed wrong with that frame, I'm in favor of dropping those frames.

Acked-by: Joerg Reuter <jreuter@xxxxxxxx>

> Cc: stable@xxxxxxxxxxxxxxx
> Cc: linux-hams@xxxxxxxxxxxxxxx
> Signed-off-by: Mashiro Chen <mashiro.chen@xxxxxxxxxxx>
> ---
>  drivers/net/hamradio/bpqether.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/hamradio/bpqether.c b/drivers/net/hamradio/bpqether.c
> index 045c5177262eaf..214fd1f819a1bb 100644
> --- a/drivers/net/hamradio/bpqether.c
> +++ b/drivers/net/hamradio/bpqether.c
> @@ -187,6 +187,9 @@ static int bpq_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_ty
>  
>  	len = skb->data[0] + skb->data[1] * 256 - 5;
>  
> +	if (len < 0 || len > skb->len - 2)
> +		goto drop_unlock;
> +
>  	skb_pull(skb, 2);	/* Remove the length bytes */
>  	skb_trim(skb, len);	/* Set the length of the data */
>  
> -- 
> 2.53.0
> 

-- 
Joerg Reuter                                    http://yaina.de/jreuter
And I make my way to where the warm scent of soil fills the evening air. 
Everything is waiting quietly out there....                 (Anne Clark)