On 7/7/2019 3:04 PM, Florian Westphal wrote:
> Paul Blakey <pa...@mellanox.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.
