netdev - Re: [Patch net] xfrm: fix reinsert in xfrm_hash

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:   Fri, 4 Jan 2019 07:58:54 +0100
From:   Florian Westphal <fw@...len.de>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     netdev@...r.kernel.org, Florian Westphal <fw@...len.de>
Subject: Re: [Patch net] xfrm: fix reinsert in xfrm_hash_rebuild()

Cong Wang <xiyou.wangcong@...il.com> wrote:
> xfrm_hash_rebuild() re-inserts existing policies into the hashtables,
> so it should not insert a same policy in the same place twice. This
> means we have to pass excl==1 to xfrm_policy_inexact_insert() and ignore
> the -EEXIST error. Otherwise we end up having an entry in the hashtable
> points to itself, which leads to a use-after-free as reported by syzbot.

Yes, double-insert is the reason for this syzbot report.

> Inside xfrm_policy_inexact_insert(), xfrm_policy_insert_list() could
> only return either a NULL pointer, a valid non-NULL pointer, or an error
> pointer (-EEXIST) when excl==1.

Right.

> Testing delpol && excl for -EEXIST
> is incorrect as it could return a valid pointer for excl case too,
> testing IS_ERR(delpol) is correct.

Agree.

>  		if (!chain) {
> -			void *p = xfrm_policy_inexact_insert(policy, dir, 0);
> +			void *p = xfrm_policy_inexact_insert(policy, dir, 1);

I am not so sure about this change.

Excl == 1 doesn't check for "this entry", it checks for any policy that
has identical properties, so I do not think its correct to pass 1 here;
only userspace can when adding entry.

Steffen, could you please hold off on this patch for a few hours?

I will finish test script updates today and will pass a series
that includes a different fix for this bug.

I will CC Cong on the patches to make sure I did not miss anything.

(Other aspect of this patch appears correct to me though).