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

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

[Alon Nafta]

Hi,

Has anyone looked at this patch?

Thanks,
Alon

On Mon, Feb 24, 2014 at 10:11 AM, Alon Nafta <alon@privatecore.com> wrote:
> From: Alon Nafta <alon@privatecore.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.
>
> 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@privatecore.com>
> Reviewed-by: Grant Grundler <grantgrundler@gmail.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;
>
>   if ((skb = de4x5_alloc_rx_buff(dev, entry, pkt_len)) == NULL) {
>       printk("%s: Insufficient memory; nuking packet.\n",
> diff -uprN -X linux-3.14-rc3/Documentation/dontdiff
> linux-3.14-rc3-orig/drivers/net/ethernet/dec/tulip/winbond-840.c
> linux-3.14-rc3/drivers/net/ethernet/dec/tulip/winbond-840.c
> --- linux-3.14-rc3-orig/drivers/net/ethernet/dec/tulip/winbond-840.c
> 2014-02-20 17:59:14.757666100 -0800
> +++ linux-3.14-rc3/drivers/net/ethernet/dec/tulip/winbond-840.c 2014-02-20
> 18:22:19.419612200 -0800
> @@ -1218,7 +1218,7 @@ static int netdev_rx(struct net_device *
>   } else {
>   struct sk_buff *skb;
>   /* Omit the four octet CRC from the length. */
> - int pkt_len = ((status >> 16) & 0x7ff) - 4;
> + int pkt_len = ((status >> 16) - 4) & 0x7ff;
>
>  #ifndef final_version
>   if (debug > 4)
> diff -uprN -X linux-3.14-rc3/Documentation/dontdiff
> linux-3.14-rc3-orig/drivers/net/ethernet/smsc/epic100.c
> linux-3.14-rc3/drivers/net/ethernet/smsc/epic100.c
> --- linux-3.14-rc3-orig/drivers/net/ethernet/smsc/epic100.c 2014-02-20
> 17:59:17.844045500 -0800
> +++ linux-3.14-rc3/drivers/net/ethernet/smsc/epic100.c 2014-02-20
> 18:21:13.196237400 -0800
> @@ -1172,7 +1172,7 @@ static int epic_rx(struct net_device *de
>   } else {
>   /* Malloc up new buffer, compatible with net-2e. */
>   /* Omit the four octet CRC from the length. */
> - short pkt_len = (status >> 16) - 4;
> + short pkt_len = ((status >> 16) - 4) & 0x7fff;
>   struct sk_buff *skb;
>
>   if (pkt_len > PKT_BUF_SZ - 4) {

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

[Eric Dumazet]

On Fri, 2014-03-21 at 18:51 -0700, Alon Nafta wrote:
> Hi,
> 
> Has anyone looked at this patch?
> 
> Thanks,
> Alon

I do not think netdev ever got a copy of it.

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

[David Laight]

From: Alon Nafta
> Hi,
> 
> Has anyone looked at this patch?
> 
> Thanks,
> Alon
> 
> On Mon, Feb 24, 2014 at 10:11 AM, Alon Nafta <alon@privatecore.com> wrote:
> > From: Alon Nafta <alon@privatecore.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@privatecore.com>
> > Reviewed-by: Grant Grundler <grantgrundler@gmail.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

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

[Alon Nafta]

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@aculab.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@privatecore.com> wrote:
>> > From: Alon Nafta <alon@privatecore.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@privatecore.com>
>> > Reviewed-by: Grant Grundler <grantgrundler@gmail.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
>
>
>

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

[David Miller]

From: Alon Nafta <alon@privatecore.com>
Date: Mon, 24 Mar 2014 09:41:21 -0700

> In regards to your comment about hardware-induced problems, from a
> holistic point of view, IMHO any buffer overflow should get fixed.

I completely disagree with you.

If we had to accomodate every possible arbitrary mis-behavior of
hardware, the drivers would be so bloated as to be nearly unusable.

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

[Alon Nafta]

Hi David,

I'm separating between HW misbehavior, and driver bugs than can lead
to code execution. Similar checks for this exact field are well
protected in other drivers, including the most common ones.

Moreover, a system can be protected from malicious HW, and in fact
doesn't have to trust HW at all (except for the CPU obviously).

On Mon, Mar 24, 2014 at 10:17 AM, David Miller <davem@davemloft.net> wrote:
> From: Alon Nafta <alon@privatecore.com>
> Date: Mon, 24 Mar 2014 09:41:21 -0700
>
>> In regards to your comment about hardware-induced problems, from a
>> holistic point of view, IMHO any buffer overflow should get fixed.
>
> I completely disagree with you.
>
> If we had to accomodate every possible arbitrary mis-behavior of
> hardware, the drivers would be so bloated as to be nearly unusable.

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

[David Miller]

From: Alon Nafta <alon@privatecore.com>
Date: Mon, 24 Mar 2014 10:35:37 -0700

> Moreover, a system can be protected from malicious HW, and in fact
> doesn't have to trust HW at all (except for the CPU obviously).

So should we check to make sure the program counter of the cpu really
increments to the next instruction every time?

There is a limit to everything.

The length field of DMA descriptors is absolutely, fully, trusted in
the vast majority of drivers.

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

[Alon Nafta]

I agree that the limit is reached where there is performance overheads
or code bloat, but since neither are introduced in this case, I'd
argue that if drivers can be made more secure by "paying" a minimal
cost, they should.

On Mon, Mar 24, 2014 at 11:35 AM, David Miller <davem@davemloft.net> wrote:
> From: Alon Nafta <alon@privatecore.com>
> Date: Mon, 24 Mar 2014 10:35:37 -0700
>
>> Moreover, a system can be protected from malicious HW, and in fact
>> doesn't have to trust HW at all (except for the CPU obviously).
>
> So should we check to make sure the program counter of the cpu really
> increments to the next instruction every time?
>
> There is a limit to everything.
>
> The length field of DMA descriptors is absolutely, fully, trusted in
> the vast majority of drivers.

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

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1553 bytes --]

On Mon, 2014-03-24 at 14:35 -0400, David Miller wrote:
> From: Alon Nafta <alon@privatecore.com>
> Date: Mon, 24 Mar 2014 10:35:37 -0700
> 
> > Moreover, a system can be protected from malicious HW, and in fact
> > doesn't have to trust HW at all (except for the CPU obviously).
> 
> So should we check to make sure the program counter of the cpu really
> increments to the next instruction every time?
> 
> There is a limit to everything.
> 
> The length field of DMA descriptors is absolutely, fully, trusted in
> the vast majority of drivers.

There is a real question as to how much peripherals should be trusted.

We already try to avoid trusting USB devices at all, because many
machines have ports that are accessible to users that shouldn't be fully
trusted.  (Also, users may assume it's just as safe to plug in a 'USB
stick' they were given, as it is to insert a disc or memory card.)  A
bug in the HID subsystem has apparently been exploited in the past for
code injection.

Given that some machines also have easily accessible hotplug PCI(e)
ports (CardBus, ExpressCard, Thunderbolt), and that those can be
distrusted at the hardware level (using an IOMMU), it seems like a
worthwhile goal to distrust them at the software level.  However, as you
hinted, this is a big job since any vulnerable driver that's installed
can be exploited.

Ben.

-- 
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
                                                           - Albert Einstein

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

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

[Alon Nafta]

I don't think they should be trusted at all, at least not to a point
where it's feasible for them to execute code on your system.
USB drivers, filesystem drivers, peripheral drivers - they're all on
the same boat, obviously having different levels of severity depending
on driver popularity.

>
> There is a real question as to how much peripherals should be trusted.
>
> We already try to avoid trusting USB devices at all, because many
> machines have ports that are accessible to users that shouldn't be fully
> trusted.  (Also, users may assume it's just as safe to plug in a 'USB
> stick' they were given, as it is to insert a disc or memory card.)  A
> bug in the HID subsystem has apparently been exploited in the past for
> code injection.
>
> Given that some machines also have easily accessible hotplug PCI(e)
> ports (CardBus, ExpressCard, Thunderbolt), and that those can be
> distrusted at the hardware level (using an IOMMU), it seems like a
> worthwhile goal to distrust them at the software level.  However, as you
> hinted, this is a big job since any vulnerable driver that's installed
> can be exploited.
>
> Ben.
>
> --
> Ben Hutchings
> Everything should be made as simple as possible, but not simpler.
>                                                            - Albert Einstein

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

[David Laight]

From: Alon Nafta 
> I don't think they should be trusted at all, at least not to a point
> where it's feasible for them to execute code on your system.
> USB drivers, filesystem drivers, peripheral drivers - they're all on
> the same boat, obviously having different levels of severity depending
> on driver popularity.

On a large number of systems any PCI or PCIe hardware has the
ability to read and write any system physical addresses.
Not only does this mean that 'dodgy' hardware can change the
kernel code, but also any attempt to restrict the memory that
the driver itself can access is likely to be circumventable.

Yes, you can raise the barrier, but there will always be low
points on it.

	David

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

[Grant Grundler]

On Tue, Mar 25, 2014 at 2:37 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Alon Nafta
>> I don't think they should be trusted at all, at least not to a point
>> where it's feasible for them to execute code on your system.
>> USB drivers, filesystem drivers, peripheral drivers - they're all on
>> the same boat, obviously having different levels of severity depending
>> on driver popularity.
>
> On a large number of systems any PCI or PCIe hardware has the
> ability to read and write any system physical addresses.
> Not only does this mean that 'dodgy' hardware can change the
> kernel code, but also any attempt to restrict the memory that
> the driver itself can access is likely to be circumventable.
>
> Yes, you can raise the barrier, but there will always be low
> points on it.

David, Alon,
your points are valid but getting a bit philosophical for me.

The original patch was trying to address *abuse* of the tulip chip and
it's not clear to me that's really feasible.  I think this patch is
"safe" in the sense it enforces correct behavior and AFAICT adds no
additional CPU cost.

If we said, "rearrange the code for better readability", would you
object to the patch?

thanks,
grant

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

[Alon Nafta]

Hi David,

Are you referring to (older) systems without IOMMU? Since IOMMU and
VT-d (for Intel) exactly solve that security problem.

On Tue, Mar 25, 2014 at 2:37 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Alon Nafta
>> I don't think they should be trusted at all, at least not to a point
>> where it's feasible for them to execute code on your system.
>> USB drivers, filesystem drivers, peripheral drivers - they're all on
>> the same boat, obviously having different levels of severity depending
>> on driver popularity.
>
> On a large number of systems any PCI or PCIe hardware has the
> ability to read and write any system physical addresses.
> Not only does this mean that 'dodgy' hardware can change the
> kernel code, but also any attempt to restrict the memory that
> the driver itself can access is likely to be circumventable.
>
> Yes, you can raise the barrier, but there will always be low
> points on it.
>
>         David
>
>
>

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

[David Laight]

From: Alon Nafta 
> Hi David,
> 
> Are you referring to (older) systems without IOMMU? Since IOMMU and
> VT-d (for Intel) exactly solve that security problem.

Yes - I don't know the details of the x86 IOMMU.

	David
 
> On Tue, Mar 25, 2014 at 2:37 AM, David Laight <David.Laight@aculab.com> wrote:
> > From: Alon Nafta
> >> I don't think they should be trusted at all, at least not to a point
> >> where it's feasible for them to execute code on your system.
> >> USB drivers, filesystem drivers, peripheral drivers - they're all on
> >> the same boat, obviously having different levels of severity depending
> >> on driver popularity.
> >
> > On a large number of systems any PCI or PCIe hardware has the
> > ability to read and write any system physical addresses.
> > Not only does this mean that 'dodgy' hardware can change the
> > kernel code, but also any attempt to restrict the memory that
> > the driver itself can access is likely to be circumventable.
> >
> > Yes, you can raise the barrier, but there will always be low
> > points on it.
> >
> >         David
> >
> >
> >

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

[Eric Dumazet]

On Tue, 2014-03-25 at 09:19 -0700, Grant Grundler wrote:
> On Tue, Mar 25, 2014 at 2:37 AM, David Laight <David.Laight@aculab.com> wrote:
> > From: Alon Nafta
> >> I don't think they should be trusted at all, at least not to a point
> >> where it's feasible for them to execute code on your system.
> >> USB drivers, filesystem drivers, peripheral drivers - they're all on
> >> the same boat, obviously having different levels of severity depending
> >> on driver popularity.
> >
> > On a large number of systems any PCI or PCIe hardware has the
> > ability to read and write any system physical addresses.
> > Not only does this mean that 'dodgy' hardware can change the
> > kernel code, but also any attempt to restrict the memory that
> > the driver itself can access is likely to be circumventable.
> >
> > Yes, you can raise the barrier, but there will always be low
> > points on it.
> 
> David, Alon,
> your points are valid but getting a bit philosophical for me.
> 
> The original patch was trying to address *abuse* of the tulip chip and
> it's not clear to me that's really feasible.  I think this patch is
> "safe" in the sense it enforces correct behavior and AFAICT adds no
> additional CPU cost.

BTW this reminds me :

 http://lkml.iu.edu//hypermail/linux/kernel/1003.3/02073.html

Some hardware have some flaws, not because its malicious but plainly
buggy, and its reasonable to take some basic checks on common 'mistakes'

Checking that the NIC doesn't pretend to receive a bigger frame than the
expected maximum seems reasonable.

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

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Mar 2014 10:26:21 -0700

> Some hardware have some flaws, not because its malicious but plainly
> buggy, and its reasonable to take some basic checks on common 'mistakes'
> 
> Checking that the NIC doesn't pretend to receive a bigger frame than the
> expected maximum seems reasonable.

That's awesome when we know that the chip in fact has such an errata.