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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1702111019110.3734@nanos>
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