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

[Mathias Krause <minipli () googlemail com>]

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.

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

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

> PaX may have decided that a huge performance
> penalty for some odd-ball drivers was worth some minor security gain
> for everybody.

PaX cares about security and security only -- not about performance in
some odd-ball driver.
The above change fixes a possible race that may lead to memory
corruption (concurrent writes to the same memory location) -- stuff
PaX cares about.

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

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


Mathias

> If there is a security angle to it I have an interest in learning what
> that is exactly. Implicit NULL'ing by the compiler?
>
> AYJ