Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sat, 11 Feb 2017 10:23:03 +0100 (CET)
From: Thomas Gleixner <tglx@...utronix.de>
To: Jess Frazelle <me@...sfraz.com>
cc: Marc Zyngier <marc.zyngier@....com>, 
    "open list:IRQ SUBSYSTEM" <linux-kernel@...r.kernel.org>, 
    kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as
 __ro_after_init



On Sat, 11 Feb 2017, Thomas Gleixner wrote:

> On Fri, 10 Feb 2017, Jess Frazelle wrote:
> 
> > Marked msi_domain_ops structs as __ro_after_init when called only during init.
> > Marked syscore_ops structs as __ro_after_init when register_syscore_ops was
> > called only during init. Most of the caller functions were already annotated as
> > __init.
> > unregister_syscore_ops() was never called on these syscore_ops.
> > This protects the data structure from accidental corruption.
> 
> Please be more careful with your changelogs. They should not start with
> telling WHAT you have done. The WHAT we can see from the patch.
> 
> The interesting information which belongs into the changelog is: WHY and
> which problem does it solve or which enhancement this is. Let me give you
> an example:
> 
>   Function pointers are a target for attacks especially when they are
>   located in statically allocated data structures. Some of these data
>   structures are only modified during init and therefor can be made read
>   only after init.
> 
>   struct msi_domain_ops can be made read only after init because they are
>   only updated in the registration case.
> 
>   struct syscore_ops can be made read only after init when they are only
>   registered, but never unregistered.
> 
> So this would be a proper change log explaning the patch.
> 
> Emphasis on WOULD, See below.
> 
> > -static struct syscore_ops irq_gc_syscore_ops = {
> > +static struct syscore_ops irq_gc_syscore_ops __ro_after_init = {
> >  	.suspend = irq_gc_suspend,
> >  	.resume = irq_gc_resume,
> >  	.shutdown = irq_gc_shutdown,
> 
> I seriously doubt that syscore_ops can be made __ro_after_init at all.
> 
> Assume the following:
> 
> last_init_function()
>   register_syscore_ops(&a_ops)
>     list_add(&a_ops->node, list);
> 
> apply_ro_after_init()
>   // a_ops are now read only
> 
> cpuhotplug happens
>   register_syscore_ops(&b_ops)
>     list_add(&b_ops->node, list);
> 
>     ===> Kernel crashes with a write access on RO memory because it tries
>     	 to link b_ops to a_ops.
> 
> The same is true for cpuhotunplug operations.

Sorry, cpuhotplug was the wrong example, but loading or unloading the KVM
modules will have that effect.

See vmx_init()/exit() and kvm_init()/exit() for reference.

Thanks,

	tglx

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.