netdev - Re: [PATCH net-next] openvswitch: prepare for stolen verdict coming from conntrack and nat engine

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: <20240703151900.GC29258@breakpoint.cc>
Date: Wed, 3 Jul 2024 17:19:00 +0200
From: Florian Westphal <fw@...len.de>
To: Aaron Conole <aconole@...hat.com>
Cc: Florian Westphal <fw@...len.de>, dev@...nvswitch.org, pshelar@....org,
	netdev@...r.kernel.org, Dumitru Ceara <dceara@...hat.com>,
	Marcelo Leitner <mleitner@...hat.com>
Subject: Re: [PATCH net-next] openvswitch: prepare for stolen verdict coming
 from conntrack and nat engine

Aaron Conole <aconole@...hat.com> wrote:
> > verdict with NF_DROP_REASON() helper,
> >
> > This helper releases the skb instantly (so drop_monitor can pinpoint
> > precise location) and returns NF_STOLEN.
> >
> > Prepare call sites to deal with this before introducing such changes
> > in conntrack and nat core.
> >
> > Signed-off-by: Florian Westphal <fw@...len.de>
> > ---
> 
> AFAIU, these changes are only impacting the existing NF_DROP cases, and
> won't impact how ovs + netfilter communicate about invalid packets.  One
> important thing to note is that we rely on:
> 
>  * Note that if the packet is deemed invalid by conntrack, skb->_nfct will be
>  * set to NULL and 0 will be returned.

Right, this is about how to communicate 'packet dropped'.

NF_DROP means 'please call kfree_skb for me'.  Problem from introspection point
of view is that drop monitor will blame nf_hook_slow() (for netfilter)
and ovs resp. act_ct for the drop.

Plan is to allow conntrack/nat engine to return STOLEN verdict ("skb
might have been free'd already").

Example change:
@@ -52,10 +53,8 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int hooknum,
        rt = skb_rtable(skb);
        nh = rt_nexthop(rt, ip_hdr(skb)->daddr);
        newsrc = inet_select_addr(out, nh, RT_SCOPE_UNIVERSE);
-       if (!newsrc) {
-               pr_info("%s ate my IP address\n", out->name);
-               return NF_DROP;
-       }
+       if (!newsrc)
+               return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EADDRNOTAVAIL);


Where NF_DROP_REASON() is:

static __always_inline int
NF_DROP_REASON(struct sk_buff *skb, enum skb_drop_reason reason, u32 err)
{
        BUILD_BUG_ON(err > 0xffff);

        kfree_skb_reason(skb, reason);

        return ((err << 16) | NF_STOLEN);
}

So drop monitoring tools will blame nf_nat_masquerade.c:nf_nat_masquerade_ipv4 and not
the consumer of the NF_DROP verdict.

I can't make such changes ATM because ovs and act_ct assume conntrack
returns only ACCEPT and DROP, so we'd get double-free.  Hope that makes
sense.

Thanks!