netdev - Re: [PATCH net-next v4 1/4] net/sched: Introduce action ct

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: <55a5a05f-b2c0-dda0-e961-75a7b4821dc1@mellanox.com>
Date:   Mon, 8 Jul 2019 07:07:55 +0000
From:   Paul Blakey <paulb@...lanox.com>
To:     Florian Westphal <fw@...len.de>
CC:     Jiri Pirko <jiri@...lanox.com>, Roi Dayan <roid@...lanox.com>,
        Yossi Kuperman <yossiku@...lanox.com>,
        Oz Shlomo <ozsh@...lanox.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Aaron Conole <aconole@...hat.com>,
        Zhike Wang <wangzhike@...com>,
        Rony Efraim <ronye@...lanox.com>,
        "nst-kernel@...hat.com" <nst-kernel@...hat.com>,
        John Hurley <john.hurley@...ronome.com>,
        Simon Horman <simon.horman@...ronome.com>,
        Justin Pettit <jpettit@....org>
Subject: Re: [PATCH net-next v4 1/4] net/sched: Introduce action ct


On 7/7/2019 3:04 PM, Florian Westphal wrote:
> Paul Blakey <paulb@...lanox.com> wrote:
>> +/* Determine whether skb->_nfct is equal to the result of conntrack lookup. */
>> +static bool tcf_ct_skb_nfct_cached(struct net *net, struct sk_buff *skb,
>> +				   u16 zone_id, bool force)
>> +{
>> +	enum ip_conntrack_info ctinfo;
>> +	struct nf_conn *ct;
>> +
>> +	ct = nf_ct_get(skb, &ctinfo);
>> +	if (!ct)
>> +		return false;
>> +	if (!net_eq(net, read_pnet(&ct->ct_net)))
>> +		return false;
>> +	if (nf_ct_zone(ct)->id != zone_id)
>> +		return false;
>> +
>> +	/* Force conntrack entry direction. */
>> +	if (force && CTINFO2DIR(ctinfo) != IP_CT_DIR_ORIGINAL) {
>> +		nf_conntrack_put(&ct->ct_general);
>> +		nf_ct_set(skb, NULL, IP_CT_UNTRACKED);
>> +
>> +		if (nf_ct_is_confirmed(ct))
>> +			nf_ct_kill(ct);
> This looks like a possible UAF:
> nf_conntrack_put() may free the conntrack entry.
>
> It seems better to do do:
> 	if (nf_ct_is_confirmed(ct))
> 		nf_ct_kill(ct);
>
> 	nf_conntrack_put(&ct->ct_general);
> 	nf_ct_set(skb, ...

Like if conntrack has just timed it out (or conntrack flushed), and skb 
holds the last ref?

thanks, will reverse the order.