Cong Wang <[email protected]> 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).
