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] [day] [month] [year] [list]

Message-ID: <CAM_iQpUrnsX3Z_8O5HMinXomXA5jVQ5G-tuXzoG-X_4JyFbrrw@mail.gmail.com>
Date:   Fri, 4 Jan 2019 20:29:26 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Florian Westphal <fw@...len.de>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [Patch net] xfrm: fix reinsert in xfrm_hash_rebuild()

On Thu, Jan 3, 2019 at 10:58 PM Florian Westphal <fw@...len.de> wrote:
>
> 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.

Yeah, I realized the hashtable could contain two identical polices
after sending out this patch.


>
> 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).

I will take a look at your fix.

Thanks.