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

[Kees Cook]

On Wed, Apr 07, 2021 at 03:50:15PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 7, 2021 at 12:49 PM Vaittinen, Matti
> <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> > 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@xxxxxxxxxxxxxxxxx> wrote:
> > > > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> > > > > On Tuesday, April 6, 2021, Matti Vaittinen <
> > > > > matti.vaittinen@xxxxxxxxxxxxxxxxx> wrote:
> 
> Kees, there are two non-security guys discussing potential security
> matters. Perhaps you may shed a light on this and tell which of our
> stuff is risky and which is not and your recommendations on it.

Hi!

> > > > > > +       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?

I may not be following the thread exactly, here, but normally the issue
is just one of robustness and code maintainability. You can't be sure all
future callers will always pass in a const string, so better to always do:

	pr_whatever("%s\n", string_var);

> > > Not a security guy, but my understanding is that this code may be
> > > used
> > > as a gadget in ROP technique of attacks.

The primary concern is with giving an attacker control over a format
string (which can be used to expose kernel memory). It used to be much
more serious when the kernel still implemented %n, which would turn such
things into a potential memory _overwrite_. We removed %n a long time
ago now. :)

> > 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");
> 
> I don't see direct issues, only indirect ones, for example, if by some
> reason the memory of this message appears writable. So, whoever
> controls the format string of printf() controls a lot. That's why it's
> preferable to spell out exact intentions in the explicit format
> string.

Right.

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

This, though, are you sure you want to use BUG()? Linus gets upset about
such things:
https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on

-- 
Kees Cook