Morning Andy,
Thanks for the review! By the way, is it me or did your mail-client
spill this out using HTML?
On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> On Tuesday, April 6, 2021, Matti Vaittinen <
> matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> > +static void die_loudly(const char *msg)
> > +{
> > + 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?
> > + BUG();
> > +}
> > +
> > +/**
> > + * regulator_irq_helper - register IRQ based regulator event/error
> > notifier
> > + *
> > + * @dev: device to which lifetime the helper's
> > lifetime is
> > + * bound.
> > + * @d: IRQ helper descriptor.
> > + * @irq: IRQ used to inform events/errors to be
> > notified.
> > + * @irq_flags: Extra IRQ flags to be OR's with the default
> > IRQF_ONESHOT
> > + * when requesting the (threaded) irq.
> > + * @common_errs: Errors which can be flagged by this IRQ for
> > all rdevs.
> > + * When IRQ is re-enabled these errors will be
> > cleared
> > + * from all associated regulators
> > + * @per_rdev_errs: Optional error flag array describing errors
> > specific
> > + * for only some of the regulators. These
> > errors will be
> > + * or'ed with common erros. If this is given
> > the array
> > + * should contain rdev_amount flags. Can be
> > set to NULL
> > + * if there is no regulator specific error
> > flags for this
> > + * IRQ.
> > + * @rdev: Array of regulators associated with this
> > IRQ.
> > + * @rdev_amount: Amount of regulators associated wit this
> > IRQ.
> > + */
> > +void *regulator_irq_helper(struct device *dev,
> > + const struct regulator_irq_desc *d, int
> > irq,
> > + int irq_flags, int common_errs, int
> > *per_rdev_errs,
> > + struct regulator_dev **rdev, int
> > rdev_amount)
> > +{
> > + struct regulator_irq *h;
> > + int ret;
> > +
> > + if (!rdev_amount || !d || !d->map_event || !d->name)
> > + return ERR_PTR(-EINVAL);
> > +
> > + 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?
> > +
> > +/**
> > + * regulator_irq_helper_cancel - drop IRQ based regulator
> > event/error notifier
> > + *
> > + * @handle: Pointer to handle returned by a successful
> > call to
> > + * regulator_irq_helper(). Will be NULLed upon
> > return.
> > + *
> > + * The associated IRQ is released and work is cancelled when the
> > function
> > + * returns.
> > + */
> > +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?
>
> > + struct regulator_irq *h = *handle;
> > +
> > + free_irq(h->irq, h);
> > + if (h->desc.irq_off_ms)
> > + cancel_delayed_work_sync(&h->isr_work);
> > +
> > + h = NULL;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(regulator_irq_helper_cancel);
> > +
> > +static void regulator_irq_helper_drop(struct device *dev, void
> > *res)
> > +{
> > + regulator_irq_helper_cancel(res);
> > +}
> > +
> > +void *devm_regulator_irq_helper(struct device *dev,
> > + const struct regulator_irq_desc
> > *d, int irq,
> > + int irq_flags, int common_errs,
> > + int *per_rdev_errs,
> > + struct regulator_dev **rdev, int
> > rdev_amount)
> > +{
> > + void **ptr;
> > +
> > + ptr = devres_alloc(regulator_irq_helper_drop, sizeof(*ptr),
> > GFP_KERNEL);
> > + if (!ptr)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + *ptr = regulator_irq_helper(dev, d, irq, irq_flags,
> > common_errs,
> > + per_rdev_errs, rdev,
> > rdev_amount);
> > +
> > + if (IS_ERR(*ptr))
> > + devres_free(ptr);
> > + else
> > + devres_add(dev, ptr);
> > +
> > + return *ptr;
>
> 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?
Best Regards
Matti Vaittinen
