On Thu, Jan 3, 2019 at 10:58 PM Florian Westphal <f...@strlen.de> wrote:
>
> Cong Wang <xiyou.wangc...@gmail.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.
