netdev - Re: [PATCH net 1/1] net: sched: cls

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: <20181007.210308.786227727970174514.davem@davemloft.net>
Date:   Sun, 07 Oct 2018 21:03:08 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     jhs@...atatu.com
Cc:     xiyou.wangcong@...il.com, jiri@...nulli.us, netdev@...r.kernel.org,
        viro@...iv.linux.org.uk
Subject: Re: [PATCH net 1/1] net: sched: cls_u32: fix hnode refcounting

From: Jamal Hadi Salim <jhs@...atatu.com>
Date: Sun,  7 Oct 2018 07:40:17 -0400

> From: Al Viro <viro@...iv.linux.org.uk>
> 
> cls_u32.c misuses refcounts for struct tc_u_hnode - it counts references
> via ->hlist and via ->tp_root together.  u32_destroy() drops the former
> and, in case when there had been links, leaves the sucker on the list.
> As the result, there's nothing to protect it from getting freed once links
> are dropped.
> That also makes the "is it busy" check incapable of catching the root
> hnode - it *is* busy (there's a reference from tp), but we don't see it as
> something separate.  "Is it our root?" check partially covers that, but
> the problem exists for others' roots as well.
> 
> AFAICS, the minimal fix preserving the existing behaviour (where it doesn't
> include oopsen, that is) would be this:
>         * count tp->root and tp_c->hlist as separate references.  I.e.
> have u32_init() set refcount to 2, not 1.
> 	* in u32_destroy() we always drop the former;
> in u32_destroy_hnode() - the latter.
> 
> 	That way we have *all* references contributing to refcount.  List
> removal happens in u32_destroy_hnode() (called only when ->refcnt is 1)
> an in u32_destroy() in case of tc_u_common going away, along with
> everything reachable from it.  IOW, that way we know that
> u32_destroy_key() won't free something still on the list (or pointed to by
> someone's ->root).
> 
> Reproducer:
 ...
> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> Signed-off-by: Jamal Hadi Salim <jhs@...atatu.com>

Applied and queued up for -stable.