netdev - Re: [PATCH net 1/2] openvswitch: Properly set L4 keys on "later" IP fragments.

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: <20190825.145246.711177960271090404.davem@davemloft.net>
Date:   Sun, 25 Aug 2019 14:52:46 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     pshelar@....org
Cc:     jpettit@....org, netdev@...r.kernel.org, joe@...d.net.nz
Subject: Re: [PATCH net 1/2] openvswitch: Properly set L4 keys on "later"
 IP fragments.

From: Pravin Shelar <pshelar@....org>
Date: Sun, 25 Aug 2019 13:40:58 -0700

> On Sun, Aug 25, 2019 at 9:54 AM Pravin Shelar <pshelar@....org> wrote:
>>
>> On Sat, Aug 24, 2019 at 9:58 AM Justin Pettit <jpettit@....org> wrote:
>> >
>> > When IP fragments are reassembled before being sent to conntrack, the
>> > key from the last fragment is used.  Unless there are reordering
>> > issues, the last fragment received will not contain the L4 ports, so the
>> > key for the reassembled datagram won't contain them.  This patch updates
>> > the key once we have a reassembled datagram.
>> >
>> > Signed-off-by: Justin Pettit <jpettit@....org>
>> > ---
>> >  net/openvswitch/conntrack.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> > index 848c6eb55064..f40ad2a42086 100644
>> > --- a/net/openvswitch/conntrack.c
>> > +++ b/net/openvswitch/conntrack.c
>> > @@ -524,6 +524,10 @@ static int handle_fragments(struct net *net, struct sw_flow_key *key,
>> >                 return -EPFNOSUPPORT;
>> >         }
>> >
>> > +       /* The key extracted from the fragment that completed this datagram
>> > +        * likely didn't have an L4 header, so regenerate it. */
>> > +       ovs_flow_key_update(skb, key);
>> > +
>> >         key->ip.frag = OVS_FRAG_TYPE_NONE;
>> >         skb_clear_hash(skb);
>> >         skb->ignore_df = 1;
>> > --
>>
>> Looks good to me.
>>
>> Acked-by: Pravin B Shelar <pshelar@....org>
>>
> Actually I am not sure about this change. caller of this function
> (ovs_ct_execute()) does skb-pull and push of L2 header, calling
> ovs_flow_key_update() is not safe here, it expect skb data to point to
> L2 header.

Agreed, also the comment needs to be formatted properly ala:

	/* blah
	 * blah
	 */

instead of:

	/* blah
	 * blah */