netdev - Re: [PATCH net] net: ensure all external references are released in deferred skbuffs

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:   Wed, 22 Jun 2022 19:03:12 +0200
From:   Eric Dumazet <edumazet@...gle.com>
To:     Ilya Maximets <i.maximets@....org>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Florian Westphal <fw@...len.de>,
        netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>, dev@...nvswitch.org,
        LKML <linux-kernel@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] net: ensure all external references are released in
 deferred skbuffs

On Wed, Jun 22, 2022 at 6:47 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> On Wed, Jun 22, 2022 at 6:39 PM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Wed, Jun 22, 2022 at 6:29 PM Eric Dumazet <edumazet@...gle.com> wrote:
> > >
> > > On Wed, Jun 22, 2022 at 4:26 PM Ilya Maximets <i.maximets@....org> wrote:
> > > >
> > > > On 6/22/22 13:43, Eric Dumazet wrote:
> > >
> > > >
> > > > I tested the patch below and it seems to fix the issue seen
> > > > with OVS testsuite.  Though it's not obvious for me why this
> > > > happens.  Can you explain a bit more?
> > >
> > > Anyway, I am not sure we can call nf_reset_ct(skb) that early.
> > >
> > > git log seems to say that xfrm check needs to be done before
> > > nf_reset_ct(skb), I have no idea why.
> >
> > Additional remark: In IPv6 side, xfrm6_policy_check() _is_ called
> > after nf_reset_ct(skb)
> >
> > Steffen, do you have some comments ?
> >
> > Some context:
> > commit b59c270104f03960069596722fea70340579244d
> > Author: Patrick McHardy <kaber@...sh.net>
> > Date:   Fri Jan 6 23:06:10 2006 -0800
> >
> >     [NETFILTER]: Keep conntrack reference until IPsec policy checks are done
> >
> >     Keep the conntrack reference until policy checks have been performed for
> >     IPsec NAT support. The reference needs to be dropped before a packet is
> >     queued to avoid having the conntrack module unloadable.
> >
> >     Signed-off-by: Patrick McHardy <kaber@...sh.net>
> >     Signed-off-by: David S. Miller <davem@...emloft.net>
> >
>
> Oh well... __xfrm_policy_check() has :
>
> nf_nat_decode_session(skb, &fl, family);
>
> This  answers my questions.
>
> This means we are probably missing at least one XFRM check in TCP
> stack in some cases.
> (Only after adding this XFRM check we can call nf_reset_ct(skb))
>

Maybe this will help ?

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index fe8f23b95d32ca4a35d05166d471327bc608fa91..49c1348e40b6c7b6a98b54d716f29c948e00ba33
100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -2019,12 +2019,19 @@ int tcp_v4_rcv(struct sk_buff *skb)
                if (nsk == sk) {
                        reqsk_put(req);
                        tcp_v4_restore_cb(skb);
-               } else if (tcp_child_process(sk, nsk, skb)) {
-                       tcp_v4_send_reset(nsk, skb);
-                       goto discard_and_relse;
                } else {
-                       sock_put(sk);
-                       return 0;
+                       if (!xfrm4_policy_check(nsk, XFRM_POLICY_IN, skb)) {
+                               drop_reason = SKB_DROP_REASON_XFRM_POLICY;
+                               goto discard_and_relse;
+                       }
+                       nf_reset_ct(skb);
+                       if (tcp_child_process(sk, nsk, skb)) {
+                               tcp_v4_send_reset(nsk, skb);
+                               goto discard_and_relse;
+                       } else {
+                               sock_put(sk);
+                               return 0;
+                       }
                }
        }