From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932086AbdBKJXI (ORCPT ); Sat, 11 Feb 2017 04:23:08 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:34978 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750988AbdBKJXH (ORCPT ); Sat, 11 Feb 2017 04:23:07 -0500 Date: Sat, 11 Feb 2017 10:23:03 +0100 (CET) From: Thomas Gleixner To: Jess Frazelle cc: Marc Zyngier , "open list:IRQ SUBSYSTEM" , kernel-hardening@lists.openwall.com Subject: Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init In-Reply-To: Message-ID: References: <20170211013758.3288-1-me@jessfraz.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Sat, 11 Feb 2017 10:23:03 +0100 (CET) From: Thomas Gleixner In-Reply-To: Message-ID: References: <20170211013758.3288-1-me@jessfraz.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Subject: [kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init To: Jess Frazelle Cc: Marc Zyngier , "open list:IRQ SUBSYSTEM" , kernel-hardening@lists.openwall.com List-ID: 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