net: tulip: xircom_cb: drop runt frames before skb copy

[PATCH] net: tulip: xircom_cb: drop runt frames before skb copy

Posted by Pablo Vallespín Aranguren 2 months ago

investigate_read_descriptor() takes the 11-bit packet length straight
from the device-written descriptor and subtracts 4 for the CRC. A
length below 4 underflows pkt_len to a negative value, which is then
passed to skb_put_data() as a huge unsigned length and corrupts memory
past the end of the freshly allocated skb.

Drop the frame as a length error if pkt_len is negative after CRC
removal.

Signed-off-by: Pablo Vallespín Aranguren <pablopva014@gmail.com>
Assisted-by: gkh_clanker_t1000
---
 drivers/net/ethernet/dec/tulip/xircom_cb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/dec/tulip/xircom_cb.c b/drivers/net/ethernet/dec/tulip/xircom_cb.c
index e5d2ede13845..62c64da1c8fa 100644
--- a/drivers/net/ethernet/dec/tulip/xircom_cb.c
+++ b/drivers/net/ethernet/dec/tulip/xircom_cb.c
@@ -1110,6 +1110,11 @@ investigate_read_descriptor(struct net_device *dev, struct xircom_private *card,
 					/* minus 4, we don't want the CRC */
 		struct sk_buff *skb;
 
+		if (pkt_len < 0) {
+			dev->stats.rx_length_errors++;
+			goto out;
+		}
+
 		if (pkt_len > 1518) {
 			netdev_err(dev, "Packet length %i is bogus\n", pkt_len);
 			pkt_len = 1518;
-- 
2.55.0

Re: [PATCH] net: tulip: xircom_cb: drop runt frames before skb copy

Posted by David Laight 2 months ago

On Fri, 31 Jul 2026 20:33:34 +0200
Pablo Vallespín Aranguren <pablopva014@gmail.com> wrote:

> investigate_read_descriptor() takes the 11-bit packet length straight
> from the device-written descriptor and subtracts 4 for the CRC. A
> length below 4 underflows pkt_len to a negative value, which is then
> passed to skb_put_data() as a huge unsigned length and corrupts memory
> past the end of the freshly allocated skb.

Have you checked that that hardware can actually set a short value?
I'd expect that packets shorter than 64 bytes (including the crc) are
dropped as 'runts' and probably don't even use a ring entry.

Of course, if you think the device might lie all bets are off.
(Without an iommu is can write anywhere in host memory...)

	David

> 
> Drop the frame as a length error if pkt_len is negative after CRC
> removal.
> 
> Signed-off-by: Pablo Vallespín Aranguren <pablopva014@gmail.com>
> Assisted-by: gkh_clanker_t1000
> ---
>  drivers/net/ethernet/dec/tulip/xircom_cb.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/dec/tulip/xircom_cb.c b/drivers/net/ethernet/dec/tulip/xircom_cb.c
> index e5d2ede13845..62c64da1c8fa 100644
> --- a/drivers/net/ethernet/dec/tulip/xircom_cb.c
> +++ b/drivers/net/ethernet/dec/tulip/xircom_cb.c
> @@ -1110,6 +1110,11 @@ investigate_read_descriptor(struct net_device *dev, struct xircom_private *card,
>  					/* minus 4, we don't want the CRC */
>  		struct sk_buff *skb;
>  
> +		if (pkt_len < 0) {
> +			dev->stats.rx_length_errors++;
> +			goto out;
> +		}
> +
>  		if (pkt_len > 1518) {
>  			netdev_err(dev, "Packet length %i is bogus\n", pkt_len);
>  			pkt_len = 1518;

Re: [PATCH] net: tulip: xircom_cb: drop runt frames before skb copy

Posted by Pablo Vallespín Aranguren 2 months ago

On Fri, Jul 31, 2026 at 08:26:29PM +0100, David Laight wrote:
> On Fri, 31 Jul 2026 20:33:34 +0200
> Pablo Vallespín Aranguren <pablopva014@gmail.com> wrote:
> 
> Have you checked that that hardware can actually set a short value?
> I'd expect that packets shorter than 64 bytes (including the crc) are
> dropped as 'runts' and probably don't even use a ring entry.

I haven't verified how the real Xircom firmware handles runt frames (I
don't own this hardware). I tested with Qemu's emulated NIC, the device
itself does validate and reject negative-size values before sending them
to the driver.

> Of course, if you think the device might lie all bets are off.
> (Without an iommu is can write anywhere in host memory...)

I took into account that the hardware could glitch or a misbehaving/fake
card could be used. I modified the emulated NIC to mimic this behaviour,
and given that the driver does not perform a check on its own (to
validate that pkt_len is not negative) it does lead to a kernel panic.

Best,

Pablo

> > Signed-off-by: Pablo Vallespín Aranguren <pablopva014@gmail.com>
> > Assisted-by: gkh_clanker_t1000
> > ---
> >  drivers/net/ethernet/dec/tulip/xircom_cb.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/dec/tulip/xircom_cb.c b/drivers/net/ethernet/dec/tulip/xircom_cb.c
> > index e5d2ede13845..62c64da1c8fa 100644
> > --- a/drivers/net/ethernet/dec/tulip/xircom_cb.c
> > +++ b/drivers/net/ethernet/dec/tulip/xircom_cb.c
> > @@ -1110,6 +1110,11 @@ investigate_read_descriptor(struct net_device *dev, struct xircom_private *card,
> >  					/* minus 4, we don't want the CRC */
> >  		struct sk_buff *skb;
> >  
> > +		if (pkt_len < 0) {
> > +			dev->stats.rx_length_errors++;
> > +			goto out;
> > +		}
> > +
> >  		if (pkt_len > 1518) {
> >  			netdev_err(dev, "Packet length %i is bogus\n", pkt_len);
> >  			pkt_len = 1518;
>

Re: [PATCH] net: tulip: xircom_cb: drop runt frames before skb copy

Posted by Jakub Kicinski 2 months ago

On Fri, 31 Jul 2026 22:56:02 +0200 Pablo Vallespín Aranguren wrote:
> > Have you checked that that hardware can actually set a short value?
> > I'd expect that packets shorter than 64 bytes (including the crc) are
> > dropped as 'runts' and probably don't even use a ring entry.  
> 
> I haven't verified how the real Xircom firmware handles runt frames (I
> don't own this hardware). I tested with Qemu's emulated NIC, the device
> itself does validate and reject negative-size values before sending them
> to the driver.

Are you related to Greg KH in any way? You use the same slop tag.
Just curious.

Please send a patch to delete this driver if you want to help.
It's ancient.

Re: [PATCH] net: tulip: xircom_cb: drop runt frames before skb copy

Posted by Greg KH 2 months ago

On Fri, Jul 31, 2026 at 04:55:55PM -0700, Jakub Kicinski wrote:
> On Fri, 31 Jul 2026 22:56:02 +0200 Pablo Vallespín Aranguren wrote:
> > > Have you checked that that hardware can actually set a short value?
> > > I'd expect that packets shorter than 64 bytes (including the crc) are
> > > dropped as 'runts' and probably don't even use a ring entry.  
> > 
> > I haven't verified how the real Xircom firmware handles runt frames (I
> > don't own this hardware). I tested with Qemu's emulated NIC, the device
> > itself does validate and reject negative-size values before sending them
> > to the driver.
> 
> Are you related to Greg KH in any way? You use the same slop tag.
> Just curious.

Pablo is part of an CS masters intern project through vu.nl that I'm
running to help audit and submit LLM-found issues against the kernel.
Hence the gkh_clanker tag, as my bot is the thing that originally
"found" this report.

> Please send a patch to delete this driver if you want to help.
> It's ancient.

No objection from me, that fixes all of the issues!

Pablo, want to submit a patch for that?

thanks,

greg k-h

Re: [PATCH] net: tulip: xircom_cb: drop runt frames before skb copy

Posted by Pablo Vallespín Aranguren 2 months ago

On Sat, Aug 01, 2026 at 09:04:27AM +0200, Greg KH wrote:
> On Fri, Jul 31, 2026 at 04:55:55PM -0700, Jakub Kicinski wrote:
> > On Fri, 31 Jul 2026 22:56:02 +0200 Pablo Vallespín Aranguren wrote:
> > 
> > Please send a patch to delete this driver if you want to help.
> > It's ancient.
> 
> No objection from me, that fixes all of the issues!
> 
> Pablo, want to submit a patch for that?

Sounds good, I will submit a patch to delete it.

Best,

Pablo