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
