From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753308AbdBKJO6 (ORCPT ); Sat, 11 Feb 2017 04:14:58 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:34970 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750988AbdBKJOz (ORCPT ); Sat, 11 Feb 2017 04:14:55 -0500 Date: Sat, 11 Feb 2017 10:14:52 +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: <20170211013758.3288-1-me@jessfraz.com> 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 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. > -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. Thanks, tglx From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Sat, 11 Feb 2017 10:14:52 +0100 (CET) From: Thomas Gleixner In-Reply-To: <20170211013758.3288-1-me@jessfraz.com> 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 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. > -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. Thanks, tglx