[kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init

[Thomas Gleixner <tglx@linutronix.de>]

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