Messages in this thread | | | Date | Sat, 11 Feb 2017 10:23:03 +0100 (CET) | From | Thomas Gleixner <> | 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
| |