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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAKPWWNFZc8v04-GM5Ttoa-CW_zhNmfMCYO2Q-Yh-75qitSUpiA@mail.gmail.com>
Date:	Mon, 24 Mar 2014 09:41:21 -0700
From:	Alon Nafta <alon@...vatecore.com>
To:	David Laight <David.Laight@...lab.com>
Cc:	netdev <netdev@...r.kernel.org>,
	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

Hi David,

Thanks for commenting on this.

In regards to your comment about hardware-induced problems, from a
holistic point of view, IMHO any buffer overflow should get fixed.
Specifically, on a locked down, DMA-protected system using any of
these drivers as its network interface, exploiting the driver remains
as one of the few practical ways of "getting in", while other HW-based
attack vectors can be mitigated.

In regards to the patch itself, I agree and was debating myself about
the specific change I should suggest. My intention was to stay as
close as possible to the original code but I can certainly cast to
unsigned instead.

Alon

On Mon, Mar 24, 2014 at 4:00 AM, David Laight <David.Laight@...lab.com> wrote:
> 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