From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753707AbdBKKtG (ORCPT ); Sat, 11 Feb 2017 05:49:06 -0500 Received: from mail-pg0-f50.google.com ([74.125.83.50]:35161 "EHLO mail-pg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752312AbdBKKtF (ORCPT ); Sat, 11 Feb 2017 05:49:05 -0500 Date: Sat, 11 Feb 2017 10:48:59 +0000 In-Reply-To: References: <20170211013758.3288-1-me@jessfraz.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Subject: Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init To: Thomas Gleixner CC: Marc Zyngier , "open list:IRQ SUBSYSTEM" , kernel-hardening@lists.openwall.com From: Jess Frazelle Message-ID: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v1BAnAPB021375 On February 11, 2017 1:14:52 AM PST, 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. 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Sat, 11 Feb 2017 10:48:59 +0000 In-Reply-To: References: <20170211013758.3288-1-me@jessfraz.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable From: Jess Frazelle Message-ID: Subject: [kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init To: Thomas Gleixner Cc: Marc Zyngier , "open list:IRQ SUBSYSTEM" , kernel-hardening@lists.openwall.com List-ID: On February 11, 2017 1:14:52 AM PST, 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=2E >> Marked syscore_ops structs as __ro_after_init when >register_syscore_ops was >> called only during init=2E Most of the caller functions were already >annotated as >> __init=2E >> unregister_syscore_ops() was never called on these syscore_ops=2E >> This protects the data structure from accidental corruption=2E > >Please be more careful with your changelogs=2E They should not start with >telling WHAT you have done=2E The WHAT we can see from the patch=2E > >The interesting information which belongs into the changelog is: WHY >and >which problem does it solve or which enhancement this is=2E Let me give >you >an example: > > Function pointers are a target for attacks especially when they are > located in statically allocated data structures=2E Some of these data > structures are only modified during init and therefor can be made read > only after init=2E > >struct msi_domain_ops can be made read only after init because they are > only updated in the registration case=2E > > struct syscore_ops can be made read only after init when they are only > registered, but never unregistered=2E > >So this would be a proper change log explaning the patch=2E Thanks for the clarification=2E > >Emphasis on WOULD, See below=2E > >> -static struct syscore_ops irq_gc_syscore_ops =3D { >> +static struct syscore_ops irq_gc_syscore_ops __ro_after_init =3D { >> =2Esuspend =3D irq_gc_suspend, >> =2Eresume =3D irq_gc_resume, >> =2Eshutdown =3D irq_gc_shutdown, > >I seriously doubt that syscore_ops can be made __ro_after_init at all=2E > >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); > > =3D=3D=3D> Kernel crashes with a write access on RO memory because it t= ries > to link b_ops to a_ops=2E > >The same is true for cpuhotunplug operations=2E This makes sense=2E Will remove=2E > >> -static struct msi_domain_ops msi_domain_ops_default =3D { >> +static struct msi_domain_ops msi_domain_ops_default __ro_after_init >=3D { > >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=2E > >msi_domain_ops_default are never ever modified, so they should be made >'const' and not __ro_after_init=2E It's not that hard to figure that out >from >the code=2E Will change to a const=2E > >Thanks, > > tglx