Re: How GNU/Linux distros deal with offset2lib attack?

[Greg KH <greg () kroah com>]

On Thu, Dec 18, 2014 at 11:36:03AM +0100, Mathias Krause wrote:
> On 18 December 2014 at 10:35, Amos Jeffries <squid3 () treenet co nz> wrote:
> > On 18/12/2014 9:24 p.m., Lionel Debroux wrote:
> > > In addition to what I wrote earlier: PaX contains several hundreds
> > > of lines of hunks dealing with local variables needlessly made
> > > static: ============================== ---
> > > linux-3.17.6/drivers/mfd/max8925-i2c.c +++
> > > linux-3.17.6-pax/drivers/mfd/max8925-i2c.c @@ -152,7 +152,7 @@
> > > static int max8925_probe(struct i2c_clie const struct i2c_device_id
> > > *id) { struct max8925_platform_data *pdata =
> > > dev_get_platdata(&client->dev); -    static struct max8925_chip
> > > *chip; +    struct max8925_chip *chip; struct device_node *node =
> > > client->dev.of_node;
> > >
> > > if (node && !pdata) {
> > >
> > > (the first reference to the "chip" variable in that function is an
> > > unconditional devm_kzalloc)
> >
> >
> > NP: I have not looked at either version of code outside the thread
> > here. Just responding to your statement of needless...
>
> Well, you better should have. It took less time to verify the bug than
> reading your comments about it.
>
> > The above sounds to me like the author wanted the alloc to only happen
> > once, lazily on first use and remain allocated until the kerel or
> > module was released. Or perhapse they wanted data in it to persist
> > between calls.
> >
> > Neither of those cases is necessarily needless. But its utility does
> > depend on how often the function is called. Saving a handful of rare
> > event allocations per kernel lifetime is almost needless (unless they
> > happen to all occur in a batch at some critical point). Saving
> > thousands per second is very much useful.
> >
> > In the former case security is best served by removing the static, in
> > the latter it is served by ensuring the struct content is fully
> > cleaned or revalidated before use in each call.
>
> All wrong. As Lionel wrote, the code assigns the variable before
> reading it. So no data is meant to persist between multiple calls to
> this function. However, if max8925_probe() gets called concurrently,
> the 'chip' pointer may change beneath one of the threads -- not good.
> So this is clearly a fix.

But that function can not be called concurrently, so this doesn't
matter.

> > - From my long experience lurking on some of the mainline dev lists ...
> > in order to get such "trivial" patches merged you will have to justify
> > that you at least considered and investigated which cases like the
> > above was the cause of the codes current form. And what the effect of
> > the proposed change would be in both the security and performance arenas.
>
> >  People using PaX code are trusting that they have done the analysis,
>
> Obviously they did.

Someone got it wrong :)

> > but that very code not being in mainline means there is possibly no
> > hard proof of that.
>
> You're wrong, again. No-one submitted the fix to LKML, that's the reason.

And if they did, they would have gotten the review I just gave.

> > > ============================== or local structs which are not meant
> > > to be modified and should therefore probably be made static /
> > > static const (mainline doesn't use the GCC plugin for
> > > constification): ============================== ---
> > > linux-3.17.6/arch/arm/mach-omap2/wd_timer.c +++
> > > linux-3.17.6-pax/arch/arm/mach-omap2/wd_timer.c @@ -110,7 +110,9 @@
> > > static int __init omap_init_wdt(void) struct omap_hwmod *oh; char
> > > *oh_name = "wd_timer2"; char *dev_name = "omap_wdt"; -    struct
> > > omap_wd_timer_platform_data pdata; +    static struct
> > > omap_wd_timer_platform_data pdata = { +        .read_reset_sources
> > > = prm_read_reset_sources +    };
> > >
> > > if (!cpu_class_is_omap2() || of_have_populated_dt()) return 0; @@
> > > -121,8 +123,6 @@ static int __init omap_init_wdt(void) return
> > > -EINVAL; }
> > >
> > > -    pdata.read_reset_sources = prm_read_reset_sources; - pdev =
> > > omap_device_build(dev_name, id, oh, &pdata, sizeof(struct
> > > omap_wd_timer_platform_data)); WARN(IS_ERR(pdev), "Can't build
> > > omap_device for %s:%s.\n", ==============================
> >
> > Now *that* does just appear to be a gratuitous cleanup / performance
> > booster. Not security related.
>
> Wrong. PaX contains a gcc plugin that does *automatic* constification
> of eligible structures (structures containing function pointers).
> That's incompatible with run-time modification of the data structures
> in question. Therefore this change fixes the incompatibility by making
> the run-time assignment a compile time constant.

Ok, that's not very obvious just looking at the patch :)

> Making structures containing function pointers r/o actually is
> security related. Read only data structures cannot be abused by memory
> corruption bugs, e.g., like the exploit for CVE-2013-2094 which
> overwrites function pointers in ptmx_fops to get code execution. But,
> well, that's true for PaX only, as write protected kernel r/o data is
> something mainline only gets when CONFIG_DEBUG_RODATA is set -- a
> '"Kernel hacking" debug option. Tells much about the state of security
> philosophy in the mainline kernel...

Cut it out with your "state of security philosophy" crap please, it
benifits no one except making you try to feel better about yourself.

If you wish to help us out with the "state of security" in the kernel,
please send patches to do so.  Participate in patch review, read all
10000 patches that get merged every 2 months and send me git ids to be
included in stable kernels, or any number of other valid, helpful things
you could do instead of just pointing at a huge, external, patchset and
saying that somehow the kernel developers (all 5000 of us I guess) have
no idea what we are doing.

greg k-h