netdev - Re: [PATCH 0/3] Check gso_size of packets when forwarding

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: <87h8rj5n6n.fsf@linkitivity.dja.id.au>
Date:   Fri, 19 Jan 2018 00:08:48 +1100
From:   Daniel Axtens <dja@...ens.net>
To:     Pravin Shelar <pshelar@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Manish.Chopra@...ium.com, ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH 0/3] Check gso_size of packets when forwarding

Pravin Shelar <pshelar@....org> writes:

> On Mon, Jan 15, 2018 at 6:09 PM, Daniel Axtens <dja@...ens.net> wrote:
>> When regular packets are forwarded, we validate their size against the
>> MTU of the destination device. However, when GSO packets are
>> forwarded, we do not validate their size against the MTU. We
>> implicitly assume that when they are segmented, the resultant packets
>> will be correctly sized.
>>
>> This is not always the case.
>>
>> We observed a case where a packet received on an ibmveth device had a
>> GSO size of around 10kB. This was forwarded by Open vSwitch to a bnx2x
>> device, where it caused a firmware assert. This is described in detail
>> at [0] and was the genesis of this series. Rather than fixing it in
>> the driver, this series fixes the forwarding path.
>>
> Are there any other possible forwarding path in networking stack? TC
> is one subsystem that could forward such a packet to the bnx2x device,
> how is that handled ?

So far I have only looked at bridges, openvswitch and macvlan. In
general, if the code uses dev_forward_skb() it should automatically be
fine as that invokes is_skb_forwardable(), which we patch.

There is potentially a forwarding path in macvlan that doesn't pass
through dev_forward_skb() (which calls is_skb_forwardable). I did post a
patch to this earlier, but it got somewhat derailed by how you would get
such a packet in to a macvlan device and whether that should be fixed
instead. Once we have some consensus on Jason's questions and the
overall approach is solid, I'm happy to work on that again.

To answer your specific question, I'm not aware of how tc can cause a
packet to be forwarded - if you can point me to the right general area I
can have a look.

Regards,
Daniel

>
>> To fix this:
>>
>>  - Move a helper in patch 1.
>>
>>  - Validate GSO segment lengths in is_skb_forwardable() in the GSO
>>    case, rather than assuming all will be well. This fixes bridges.
>>    This is patch 2.
>>
>>  - Open vSwitch uses its own slightly specialised algorithm for
>>    checking lengths. Wire up checking for that in patch 3.
>>
>> [0]: https://patchwork.ozlabs.org/patch/859410/
>>
>> Cc: Manish.Chopra@...ium.com
>> Cc: dev@...nvswitch.org
>>
>> Daniel Axtens (3):
>>   net: move skb_gso_mac_seglen to skbuff.h
>>   net: is_skb_forwardable: validate length of GSO packet segments
>>   openvswitch: drop GSO packets that are too large
>>
>>  include/linux/skbuff.h  | 16 ++++++++++++++++
>>  net/core/dev.c          |  7 ++++---
>>  net/core/skbuff.c       | 34 ++++++++++++++++++++++++++++++++++
>>  net/openvswitch/vport.c | 37 ++++++++++++++++++++++++++++++-------
>>  net/sched/sch_tbf.c     | 10 ----------
>>  5 files changed, 84 insertions(+), 20 deletions(-)
>>
>> --
>> 2.14.1
>>