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