linux-kernel - Re: [PATCH] clk: check ops pointer on clock register

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: <1513631484.29566.9.camel@baylibre.com>
Date:   Mon, 18 Dec 2017 22:11:24 +0100
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Stephen Boyd <sboyd@...eaurora.org>,
        Michael Turquette <mturquette@...libre.com>
Cc:     linux-clk <linux-clk@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] clk: check ops pointer on clock register

On Mon, 2017-12-18 at 13:06 -0800, Stephen Boyd wrote:
> On 12/18, Michael Turquette wrote:
> > Hi Jerome & Stephen,
> > 
> > On Mon, Dec 18, 2017 at 12:06 PM, Jerome Brunet <jbrunet@...libre.com> wrote:
> > > On Mon, 2017-12-18 at 11:03 -0800, Stephen Boyd wrote:
> > > > On 12/18, Jerome Brunet wrote:
> > > > > Nothing really prevents a provider from (trying to) register a clock
> > > > > without providing the clock ops structure.
> > > > > 
> > > > > We do check the individual fields before using them, but not the
> > > > > structure pointer itself. This may have the usual nasty consequences when
> > > > > the pointer is dereferenced, mostly likely when checking one the field
> > > > > during the initialization.
> > > > 
> > > > Yes, that nasty consequence should be a kernel oops,
> > > 
> > > Precisely
> > > 
> > > > and the
> > > > developer should notice that before submitting the driver for
> > > > inclusion.
> > > 
> > > Agreed. But people may make mistakes, which is why (at least partly) we
> > > do checks, isn't it ?
> > 
> > Agreed the developers should test before submitting, but procedurally
> > generated clocks (e.g. registering clocks in a loop using a
> > predictable register map, etc) could lead to a situation where a
> > developer doesn't test every possible iteration.
> > 
> > Hypothetical, but easy easy easy to fix with Jerome's patch.
> > 
> > > 
> > > > I don't think we really care to return an error here
> > > > if this happens.
> > > > 
> > > 
> > > I don't understand why we would let a oops happen when can catch the error
> > > properly ?
> > > 
> > 
> > Agreed with Jerome on this one.
> > 
> > Let's flip it on its head: any downside to this patch? If not I can merge.
> > 
> 
> If code is not checking return values from clk_register(), then
> an oops turns into a silently ignored error. 

It would really be asking for trouble, wouldn't it ?

> Hunting that down is
> going to take some time vs. an oops when we attempt to call the
> clk ops that aren't there.
> 
> The idea is fine, but I would change two things. First I would
> throw a WARN_ON() around the condition so developers notice
> quicker that something is wrong, and second I would strip off the
> 'Fixes' tag because this isn't really fixing anything that we
> need to backport to stable trees. It just converts an oops into a
> warning.
> 

Fine by me.