netdev - Re: [PATCH RFC 0/5] net/sched: validate the control action with all the other parameters

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: <7adbb8cbf7cbc5bced0e3235d3ca10faa29b55c3.camel@redhat.com>
Date:   Tue, 19 Feb 2019 17:51:12 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Vlad Buslov <vladbu@...lanox.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC 0/5] net/sched: validate the control action with
 all the other parameters

On Mon, 2019-02-18 at 22:42 -0800, Cong Wang wrote:
> On Fri, Feb 15, 2019 at 3:06 PM Davide Caratti <dcaratti@...hat.com> wrote:
> > currently, the kernel checks for bad values of the control action in
> > tcf_action_init_1(), after a successful call to the action's init()
> > function. This causes three bad behaviors:
> 
> Yeah, I have been complaining about this for a long time,
> although slightly differently. The problem here is the lack of
> "copy" in RCU mechanism, which makes it nearly impossible
> to rollback to the previous state of an action on failure path
> of an update, which also makes RCU readers reading a partially
> updated action too.

thanks for looking at this code.

by the way, I see that act_mirred has an error path after the
assignment of tcfm_eaction and tcfa_action, and this is again causing
a fail in the 'replace with bad action' tests ('half write', issue #1).
Since it's the same problem, I will fix this in the same patch (moving the
assignment after the 'if' test on the value of parm->ifindex.

> Before I fix the "copy" part, your fixes make sense to me. There
> might be some other way to expose the action-specific tcfa_action
> opcode, but it might not be better than yours.
> 
> BTW, please fold these bad behaviors into each appropriate
> patch, and keep the cover letter as an overview of the whole
> patchset rather than showing any details.
> 
> [...]

Ok, and I plan to add a selftest for each action - so that it's possible
to verify functionality (at least problem #1) before and after each
ommit.

> > all these three problems can be fixed if we validate the control action
> > in the init() function, in the same way as we are already doing for all
> > the other parameters.
> > 
> > - patch 1 is a temporary fix for problem 2), but it's reverted at the
> >   end of the series
> 
> Please drop patch 1, it is very unlikely only patch 1 will be backported,
> I think the whole patchset should be, therefore we have no reason
> to carry a temporary fix here.

sure, I will do that.
thanks!

-- 
davide