netdev - Re: [PATCH] bnx2x: drop packets where gso

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]

Date:   Mon, 18 Sep 2017 14:41:51 +1000
From:   Daniel Axtens <dja@...ens.net>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     netdev@...r.kernel.org, tlfalcon@...ux.vnet.ibm.com,
        Yuval.Mintz@...ium.com, ariel.elior@...ium.com,
        everest-linux-l2@...ium.com, jay.vosburgh@...onical.com
Subject: Re: [PATCH] bnx2x: drop packets where gso_size is too big for hardware

Hi Eric,

>> +		if (unlikely(skb_shinfo(skb)->gso_size + hlen > MAX_PACKET_SIZE)) {
>> +			BNX2X_ERR("reported gso segment size plus headers "
>> +				  "(%d + %d) > MAX_PACKET_SIZE; dropping pkt!",
>> +				  skb_shinfo(skb)->gso_size, hlen);
>> +
>> +			goto free_and_drop;
>> +		}
>> +
>
>
> If you had this test in bnx2x_features_check(), packet could be
> segmented by core networking stack before reaching bnx2x_start_xmit() by
> clearing NETIF_F_GSO_MASK
>
> -> No drop would be involved.
>
> check i40evf_features_check() for similar logic.

So I've been experimenting with this and reading through the core
networking code. If my understanding is correct, disabling GSO will
cause the packet to be segmented, but it will be segemented into
gso_size+header length packets. So in this case (~10kB gso_size) the
resultant packets will still be too big - although at least they don't
cause a crash in that case.

We could continue with this anyway as it at least prevents the crash -
but, and I haven't been able to find a nice definitive answer to this -
are implementations of ndo_start_xmit permitted to assume that the the
skb passed in will fit within the MTU? I notice that most callers will
attempt to ensure this - for example ip_output.c, ip6_output.c and
ip_forward.c all contain calls to skb_gso_validate_mtu(). If
implementations are permitted to assume this, perhaps a fix to
openvswitch would be more appropriate?

Regards,
Daniel