[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DF37E0AB-7310-4D1C-A710-7A7EA60498E7@jessfraz.com>
Date: Sat, 11 Feb 2017 10:48:59 +0000
From: Jess Frazelle <me@...sfraz.com>
To: Thomas Gleixner <tglx@...utronix.de>
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 February 11, 2017 1:14:52 AM PST, Thomas Gleixner <tglx@...utronix.de> 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.
Thanks for the clarification.
>
>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.
This makes sense. Will remove.
>
>> -static struct msi_domain_ops msi_domain_ops_default = {
>> +static struct msi_domain_ops msi_domain_ops_default __ro_after_init
>= {
>
>This is pointless and just tells me that you did a mechanical search
>for
>these ops and then blindly added __ro_after_init instead of analysing
>how
>msi_domain_ops_default is used.
>
>msi_domain_ops_default are never ever modified, so they should be made
>'const' and not __ro_after_init. It's not that hard to figure that out
>from
>the code.
Will change to a const.
>
>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.
