Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

Hello Andy,

On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote:
> On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> > 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à là, besides build bot complaints, this has serious security
> > > 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.

Thanks Andy. It'd be interesting to learn more details as I am not a
security expert either :)

> 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.

Yes. This is a good reason to not to do this - but I was interested in
knowing if there is a potential risk even if:

> > all the callers use this
> > as
> > 
> > die_loudly("foobarfoo\n");


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

Yes yes. As I said, I am not trying to say it is Ok. I was just
wondering what are the risks if users of the print function were known.

> 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.

Thanks anyways :)

> 
> > > > +       BUG();
> > > > +}
> > > > +
> 
> ...
> 
> > > > errors will be
> > > > + *                     or'ed with common erros. If this is
> > > > given
> 
> errors ?

Thanks. I didn't first spot the typo even though you pointed it to me.
Luckily my evolution has occasional problems when communicating with
the mail server. I had enough time to hit the cancel before sending out
a message where I wondered how I should clarify this :]

> ...
> 
> > > > +       if (irq <= 0) {
> > > > +               dev_err(dev, "No IRQ\n");
> > > > +               return ERR_PTR(-EINVAL);
> > > 
> > > Why shadowing error code? Negative IRQ is anything but “no IRQ”.
> > 
> > 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).

Yep. I was going to drop the check. Good point. Thanks.
I'll send v6 shortly to address the issues you spotted Andy. Thanks.

> 
> ...
> 
> > > > +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
> present.

I think I'll keep the check unless there is some reason why it should
be omitted.

> > > 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.

Right. This is probably the reason why they were written as they are. I
was just wondering if Mark had a reason to keep them that way - or if
he would appreciate it if one converted them to use the
devm_add_action() family of functions.

Best Regards
  Matti.