netdev - Re: ixgbevf: suspicious skb->pkt

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] [day] [month] [year] [list]

Message-ID: <52B35888.9050506@intel.com>
Date:	Thu, 19 Dec 2013 12:35:20 -0800
From:	John Fastabend <john.r.fastabend@...el.com>
To:	Florian Fainelli <f.fainelli@...il.com>
CC:	netdev <netdev@...r.kernel.org>, e1000-devel@...ts.sourceforge.net,
	jeffrey.t.kirsher@...el.com
Subject: Re: ixgbevf: suspicious skb->pkt_type check

On 12/19/2013 10:08 AM, Florian Fainelli wrote:
> Hi John,
>
> In commit 815cccbf ("ixgbe: add setlink, getlink support to ixgbe and
> ixgbevf") this line was added:
>
>
> +               if ((skb->pkt_type & (PACKET_BROADCAST | PACKET_MULTICAST)) &&
> +                   !(compare_ether_addr(adapter->netdev->dev_addr,
> +                                       eth_hdr(skb)->h_source))) {
>
> This looks suspicious to me since skb->pkt_type is not a bitmask, but
> holds only one value at a time,  and with this check you would also
> match any value which has the lower two bits set, so PACKET_OTHERHOST,
> PACKET_LOOPBACK and PACKET_FASTROUTE are also matching the first part
> of the check.
>

Yep, it is poorly coded I'll send a fix to Jeff to make it readable.

Note it doesn't actually break anything in practice because the only
types that can be set at this point are broadcast, multicast or
otherhost. And because this virtual function is behind the embedded
switch in the nic only packets with the correct destination address
or multicast addresses will make it to the virtual function. The
virtual function doesn't currently support promisc mode further promisc
mode in the context of a switch that doesn't support flooding is sort
of non-sense to start with. All that said a much better check would
be 'if (skb->pkt_type && ...) ' thanks for catching it.

Thanks,
John
--
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