netdev - Re: [PATCH net 1/2] net/sched: cls_u32: fix refcount leak in the error path of u32

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]

Message-ID: <bafb52ff-1ced-91a4-05d0-07d3fdc4f3e4@mojatatu.com>
Date:   Wed, 18 Dec 2019 09:23:50 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Davide Caratti <dcaratti@...hat.com>, netdev@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>
Cc:     Vlad Buslov <vladbu@...lanox.com>, Roman Mashak <mrv@...atatu.com>
Subject: Re: [PATCH net 1/2] net/sched: cls_u32: fix refcount leak in the
 error path of u32_change()

On 2019-12-17 6:00 p.m., Davide Caratti wrote:
> when users replace cls_u32 filters with new ones having wrong parameters,
> so that u32_change() fails to validate them, the kernel doesn't roll-back
> correctly, and leaves semi-configured rules.
> 
> Fix this in u32_walk(), avoiding a call to the walker function on filters
> that don't have a match rule connected. The side effect is, these "empty"
> filters are not even dumped when present; but that shouldn't be a problem
> as long as we are restoring the original behaviour, where semi-configured
> filters were not even added in the error path of u32_change().
> 
> Fixes: 6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
> Signed-off-by: Davide Caratti <dcaratti@...hat.com>

Hi Davide,

Great catch (and good test case addition),
but I am not sure about the fix.

Unless I am  misunderstanding the flow is:
You enter bad rules, validation fails, partial state had already been
created (in this case root hts) before validation failure, you then
leave the partial state in the kernel but when someone dumps you hide
these bad tables?

It sounds like the root cause is there is a missing destroy()
invocation somewhere during the create/validation failure - which is
what needs fixing... Those dangling tables should not have been
inserted; maybe somewhere along the code path for tc_new_tfilter().
Note "replace" is essentially "create if it doesnt
exist" semantic therefore NLM_F_CREATE will be set.

Since this is a core cls issue - I would check all other classifiers
with similar aggrevation - make some attribute fail in the middle or
end.
Very likely only u32 is the victim.

cheers,
jamal