Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1617690965.git.matti.vaittinen@fi.rohmeurope.com>
 <2b87b4637fde2225006cc122bc855efca0dcd7f1.1617692184.git.matti.vaittinen@fi.rohmeurope.com>
 <CAHp75VeoTVNDemV0qRA4BTVqOVfyR9UKGWhHgfeat8zVVGcu_Q@mail.gmail.com> <55397166b1c4107efc2a013635f63af142d9b187.camel@fi.rohmeurope.com>
In-Reply-To: <55397166b1c4107efc2a013635f63af142d9b187.camel@fi.rohmeurope.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 7 Apr 2021 12:10:43 +0300
Message-ID: <CAHp75VeK+Oq9inOLcSSsq+FjaaPC5D=EMt4vLf97uR1BmpW2Zw@mail.gmail.com>
Subject: Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
To:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-power@fi.rohmeurope.com" <linux-power@fi.rohmeurope.com>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "linux-renesas-soc@vger.kernel.org" 
        <linux-renesas-soc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> Morning Andy,
>
> Thanks for the review! By the way, is it me or did your mail-client
> spill this out using HTML?

It's Gmail from my mobile phone, sorry for that. We have to blame
Google that they don't think through.

> On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> > On Tuesday, April 6, 2021, Matti Vaittinen <
> > matti.vaittinen@fi.rohmeurope.com> wrote:

...

> > > +       pr_emerg(msg);
> >
> > Oh l=C3=A0 l=C3=A0, besides build bot complaints, this has serious secu=
rity
> > implications. Never do like this.
>
> I'm not even trying to claim that was correct. And I did send a fixup -
> sorry for this. I don't intend to do this again.
>
> Now, when this is said - If you have a minute, please educate me.
> Assuming we know all the callers and that all the callers use this as
>
> die_loudly("foobarfoo\n");
> - what is the exploit mechanism?

Not a security guy, but my understanding is that this code may be used
as a gadget in ROP technique of attacks.
In that case msg can be anything. On top of that, somebody may
mistakenly (inadvertently) put the code that allows user controller
input to go to this path.

And last but not least, that some newbies might copy'n'paste bad
examples where they will expose security breach.

With the modern world of Spectre, rowhammer, and other side channel
attacks I may believe that one may exhaust the regulator for getting
advantage on an attack vector.

But again, not a security guy here.

> > > +       BUG();
> > > +}
> > > +

...

> > > errors will be
> > > + *                     or'ed with common erros. If this is given

errors ?

...

> > > +       if (irq <=3D 0) {
> > > +               dev_err(dev, "No IRQ\n");
> > > +               return ERR_PTR(-EINVAL);
> >
> > Why shadowing error code? Negative IRQ is anything but =E2=80=9Cno IRQ=
=E2=80=9D.
>
> This was a good point. The irq is passed here as parameter. From this
> function's perspective the negative irq is invalid parameter - we don't
> know how the caller has obtained it. Print could show the value
> contained in irq though.

> Now that you pointed this out I am unsure if this check is needed here.
> If we check it, then I still think we should report -EINVAL for invalid
> parameter. Other option is to just call the request_threaded_irq() -
> log the IRQ request failure and return what request_threaded_irq()
> returns. Do you think that would make sense?

Why is the parameter signed type then?
Shouldn't the caller take care of it?

Otherwise, what is the difference between passing negative IRQ to
request_irq() call?
As you said, you shouldn't make assumptions about what caller meant by this=
.

So, I would simply drop the check (from easiness of the code perspective).

...

> > > +void regulator_irq_helper_cancel(void **handle)
> > > +{
> > > +       if (handle && *handle) {
> >
> > Can handle ever be NULL here ? (Yes, I understand that you export
> > this)
>
> To tell the truth - I am not sure. I *guess* that if we allow this to
> be NULL, then one *could* implement a driver for IC where IRQs are
> optional, in a way that when IRQs are supported the pointer to handle
> is valid, when IRQs aren't supported the pointer is NULL. (Why) do you
> think we should skip the check?

Just my guts feeling. I don't remember that I ever saw checks like
this for indirect pointers.
Of course it doesn't mean there are no such checks present or may be presen=
t.

...

> > Why not to use devm_add_action{_or_reset}()?
>
> I just followed the same approach that has been used in other regulator
> functions. (drivers/regulator/devres.c)
> OTOH, the devm_add_action makes this little bit simpler so I'll convert
> to use it.
>
> Mark, do you have a reason of not using devm_add_action() in devres.c?
> Should devm_add_action() be used in some other functions there? And
> should this be moved to devres.c?

I think the reason for this is as simple as a historical one, i.e.
there was no such API that time.

--=20
With Best Regards,
Andy Shevchenko