[PATCH net 0/2] net: hamradio: fix missing input validation in bpqether and scc

[PATCH net 0/2] net: hamradio: fix missing input validation in bpqether and scc

[Mashiro Chen]

Two fixes for missing input validation in the hamradio drivers:

- bpqether: bpq_rcv() computes frame length as data[0] + data[1]*256 - 5,
  which can underflow when the length fields encode a value less than 5.
  The resulting negative value is subsequently used as an unsigned length,
  leading to out-of-bounds access.

- scc: the SIOCSCCSMEM ioctl accepts a bufsize of 0 without validation.
  When a receive interrupt fires, dev_alloc_skb(0) allocates an skb with
  an empty data area, and the subsequent skb_put_u8() calls write into
  the adjacent skb_shared_info, corrupting heap memory.

Both fixes are minimal, adding only a bounds check before the dangerous
operation.

Mashiro Chen (2):
  net: hamradio: bpqether: validate frame length in bpq_rcv()
  net: hamradio: scc: validate bufsize in SIOCSCCSMEM ioctl

 drivers/net/hamradio/bpqether.c | 3 +++
 drivers/net/hamradio/scc.c      | 2 ++
 2 files changed, 5 insertions(+)

-- 
2.53.0

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

[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.

Additionally, a negative len corrupts the 64-bit rx_bytes counter
through implicit sign-extension.

Add a bounds check before pulling the length bytes: reject frames
where len is negative or exceeds the remaining skb data.

Cc: stable@vger.kernel.org
Cc: linux-hams@vger.kernel.org
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 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

[PATCH net 2/2] net: hamradio: scc: validate bufsize in SIOCSCCSMEM ioctl

[Mashiro Chen]

The SIOCSCCSMEM ioctl copies a scc_mem_config from user space and
assigns its bufsize field directly to scc->stat.bufsize without any
range validation:

  scc->stat.bufsize = memcfg.bufsize;

If a privileged user (CAP_SYS_RAWIO) sets bufsize to 0, the receive
interrupt handler later calls dev_alloc_skb(0) and immediately writes
a KISS type byte via skb_put_u8() into a zero-capacity socket buffer,
corrupting the adjacent skb_shared_info region.

The scc.c comment already states the buffer must not exceed 4096 bytes,
but this limit is never enforced.  Add a bounds check that rejects values
outside the range [16, 4096], consistent with the documented constraint
and large enough to hold at least one KISS header byte plus useful data.

Cc: stable@vger.kernel.org
Cc: linux-hams@vger.kernel.org
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 drivers/net/hamradio/scc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/hamradio/scc.c b/drivers/net/hamradio/scc.c
index ae5048efde686a..fd3ff3f4311df2 100644
--- a/drivers/net/hamradio/scc.c
+++ b/drivers/net/hamradio/scc.c
@@ -1909,6 +1909,8 @@ static int scc_net_siocdevprivate(struct net_device *dev,
 			if (!capable(CAP_SYS_RAWIO)) return -EPERM;
 			if (!arg || copy_from_user(&memcfg, arg, sizeof(memcfg)))
 				return -EINVAL;
+			if (memcfg.bufsize < 16 || memcfg.bufsize > 4096)
+				return -EINVAL;
 			scc->stat.bufsize   = memcfg.bufsize;
 			return 0;
 		
-- 
2.53.0

Re: [PATCH net 2/2] net: hamradio: scc: validate bufsize in SIOCSCCSMEM ioctl

[Joerg Reuter]

Hi,

Am Thu, Apr 09, 2026 at 01:23:58AM +0800 schrieb Mashiro Chen:

> If a privileged user (CAP_SYS_RAWIO) sets bufsize to 0, the receive
> interrupt handler later calls dev_alloc_skb(0) and immediately writes
> a KISS type byte via skb_put_u8() into a zero-capacity socket buffer,
> corrupting the adjacent skb_shared_info region.

Oops, that's unfortunate.

> The scc.c comment already states the buffer must not exceed 4096 bytes,
> but this limit is never enforced.

That was a limit 30 years ago when we couldn't have skbs larger than one
page.

I'm not sure if anyone is actually using AX.25 jumbograms with a Zilog SCC
controller, that doesn't make much sense to me. But maybe someone out there
is indeed running IP over huge AX.25 UI frames, thus I'm not a fan of
enforcing an upper limit either. It's hamradio, you're supposed to tinker.

I'm okay with a mininum size of 16, of course.

73,
    Joerg

-- 
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)

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@yaina.de>

> Cc: stable@vger.kernel.org
> Cc: linux-hams@vger.kernel.org
> Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
> ---
>  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)