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: <CAEG3pNBi13BCPcftf=mj5LJh3JQQi+Q7C5QLZwoy6LG__krJXA@mail.gmail.com>
Date:   Mon, 18 Dec 2017 12:12:51 -0800
From:   Michael Turquette <mturquette@...libre.com>
To:     Jerome Brunet <jbrunet@...libre.com>
Cc:     Stephen Boyd <sboyd@...eaurora.org>,
        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

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.

Regards,
Mike