ixgbevf: suspicious skb->pkt_type check

ixgbevf: suspicious skb->pkt_type check

[Florian Fainelli]

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.
-- 
Florian

Re: ixgbevf: suspicious skb->pkt_type check

[John Fastabend]

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