netdev - Re: [PATCH net-next] net/sched: fix a couple of splats in the error path of tfc_gate

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: <696c630f8c72f2a6a0674b69921fd500f1d5d4d1.camel@redhat.com>
Date:   Fri, 29 May 2020 20:08:26 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Po Liu <po.liu@....com>, Jamal Hadi Salim <jhs@...atatu.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Ivan Vecera <ivecera@...hat.com>
Subject: Re: [PATCH net-next] net/sched: fix a couple of splats in the
 error path of tfc_gate_init()

hi Po Liu,

On Fri, 2020-05-29 at 02:43 +0000, Po Liu wrote:
> Can you share the test step? 

sure, an invalid value of the control action is sufficient:

# tc action add action gate index 2 clockid CLOCK_TAI goto chain 42

> Clockid by default is set with CLOCK_TAI.

not in the error path of tcf_gate_init(), see below:

> And INIT_LIST_HEAD() also called in the init.

...ditto. In the error path of tcf_gate_init(), these two initializations
are not done. Looking at the call trace, validation of the control action
fails here:

365         err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
366         if (err < 0)
367                 goto release_idr;

then, the execution jumps to 'release_idr' thus skipping INIT_LIST_HEAD()
and hrtimer_init():

442 release_idr:
443         tcf_idr_release(*a, bind);
444         return err;

because of this, tcf_gate_cleanup() is invoked with

	'to_gate(*a)->param.entries'

all filled with zeros, and the same applies to

	'to_gate(*a)->hitimer'

and 

	'to_gate(*a)->param.tfcg_clockid'

> So I think maybe there is better method to avoid the duplicated code.

I'm not sure of what duplication you are referring to, but I suspect it's
those to_gate(*a) inside the if (ret == ACT_P_CREATED) { ... } statement:
I'm sending right now a v2 where I moved the assignment of 'gact' earlier.

Looking again at the error path of tcf_gate_init(), I suspect there is
another bug: the validation of 'tcfg_cycletime' and 'TCA_GATE_ENTRY_LIST'
is suspicious, because it overwrites the action's configuration with wrong
ones, thus causing semi-configured rules.
But it's unrelated to this kernel panic, so probably it deserves a
separate patch (and moreover, I don't have yet scripts that to verify it).
But I can follow-up on this in the next days, if you want.

thanks for looking at this,
-- 
davide