netdev - RE: [PATCH 1/4 V2] Ethernet drivers in 3.14-rc3 kernel: fix 3 buffer overflows triggered by hardware devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <063D6719AE5E284EB5DD2968C1650D6D0F6E77C0@AcuExch.aculab.com>
Date:	Mon, 24 Mar 2014 11:00:17 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Alon Nafta' <alon@...vatecore.com>,
	netdev <netdev@...r.kernel.org>
CC:	Grant Grundler <grantgrundler@...il.com>
Subject: RE: [PATCH 1/4 V2] Ethernet drivers in 3.14-rc3 kernel: fix 3
 buffer overflows triggered by hardware devices

From: Alon Nafta
> Hi,
> 
> Has anyone looked at this patch?
> 
> Thanks,
> Alon
> 
> On Mon, Feb 24, 2014 at 10:11 AM, Alon Nafta <alon@...vatecore.com> wrote:
> > From: Alon Nafta <alon@...vatecore.com>
> >
> > Linux Kernel 3.14 contains multiple overflow conditions that are triggered
> > as hardware inputs are not properly validated when parsing Ethernet packets.
> > This may allow a local attacker to cause an overflow, resulting in a denial
> > of service or potentially allowing the execution of arbitrary code.

These are lengths written by hardware, so will only be wrong if the
hardware is broken.
If the hardware is broken (or replaced by something malicious)
then it can do anything it likes.
Invalid values in ring entries are the least of your worries.

It also isn't clear that generating an overlong packet is
any better.

> > The programmatic error resides in the use of an integer type to describe
> > packet lengths, without proper validation to disallow negative values. In
> > all three (3) bugs this patch fixes, a value of 0x30000 of the hardware
> > signal, named status, will result in a value of 0xffffffff for pkt_len, and
> > an allocation of a socket buffer with size of 0x1. This results in an
> > overflow when data is copied into that buffer.
> >
> > Signed-off-by: Alon Nafta <alon@...vatecore.com>
> > Reviewed-by: Grant Grundler <grantgrundler@...il.com>
> > ---
> > diff -uprN -X linux-3.14-rc3/Documentation/dontdiff
> > linux-3.14-rc3-orig/drivers/net/ethernet/dec/tulip/de4x5.c
> > linux-3.14-rc3/drivers/net/ethernet/dec/tulip/de4x5.c
> > --- linux-3.14-rc3-orig/drivers/net/ethernet/dec/tulip/de4x5.c 2014-02-20
> > 17:59:14.704084300 -0800
> > +++ linux-3.14-rc3/drivers/net/ethernet/dec/tulip/de4x5.c 2014-02-20
> > 18:23:08.987749400 -0800
> > @@ -1635,8 +1635,8 @@ de4x5_rx(struct net_device *dev)
> >   if (status & RD_OF)           lp->pktStats.rx_overflow++;
> >       } else {                          /* A valid frame received */
> >   struct sk_buff *skb;
> > - short pkt_len = (short)(le32_to_cpu(lp->rx_ring[entry].status)
> > -                             >> 16) - 4;
> > + short pkt_len = (short)((le32_to_cpu(lp->rx_ring[entry].status)
> > +                             >> 16) - 4) & 0x7fff;

There isn't much point masking with 0x7fff and then casting to (short).
Actually changing the variable to 'unsigned int' might generate
better code.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html