linux-kernel - Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1702161536150.3543@nanos>
Date:   Thu, 16 Feb 2017 15:38:05 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Bjorn Helgaas <helgaas@...nel.org>
cc:     Jess Frazelle <me@...sfraz.com>,
        "K. Y. Srinivasan" <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Keith Busch <keith.busch@...el.com>,
        "open list:Hyper-V CORE AND DRIVERS" <devel@...uxdriverproject.org>,
        "open list:PCI SUBSYSTEM" <linux-pci@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        kernel-hardening@...ts.openwall.com,
        Kees Cook <keescook@...omium.org>,
        Marc Zyngier <marc.zyngier@....com>
Subject: Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init

On Thu, 16 Feb 2017, Bjorn Helgaas wrote:
> On Wed, Feb 15, 2017 at 10:16:32PM +0100, Thomas Gleixner wrote:
> 
> > I think I suggested to Jiang to do that 'update with default functions' to
> > 
> > - avoid exporting the world and some more
> > 
> > - have the flexibility to add new functions to the ops w/o updating a
> >   gazillion of existing usage sites, which has saved us lots of chaising in
> >   the last years
> > 
> > - avoid the if (ops->ptr) ops->ptr(); else default_fn(); constructs all
> >   over the place.
> > 
> > I admit I did not think about the fact that this makes the structs non
> > const.
> > 
> > Mopping that up by exporting the default functions and setting all the
> > function pointers is tedious and requires a full tree sweep when we add new
> > stuff. There's also code shared between PCI/platform/DT based stuff, so
> > that becomes interesting.
> 
> It's legal to initialize a field multiple times, and the last one
> takes precedence, so doing this might at least avoid the full tree
> sweeps:
> 
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     MSI_DOMAIN_DEFAULT_OPS,
>     .get_hwirq = vmd_get_hwirq,
>   };
> 
> The functions referenced by MSI_DOMAIN_DEFAULT_OPS would still have to
> be exported, though.

Hmm, that'd work. Though it will fall apart for those pieces where we share
code across backends. But I did not yet go through all the places and check
them.

> > Doing the if (ops->ptr) ops->ptr() else default_fn(); dance should be
> > simpler to pull off. There are not that many sites to look at, but then we
> > have some of the GICv3 code using the domain ops out of core.
> > 
> > For now doing the __ro_after_init is definitely the simplest and fastest
> > solution to tighten these statically allocated structures.
> 
> I'm OK with __ro_after_init, at least as an interim solution.
> 
> I do think it would be good to audit all the uses of
> MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS, since they
> seem to be the primary indicator of when the struct might be modified.
> I suspect we could add __ro_after_init to more than just pci-hyperv.c,
> vmd.c, and msi.c

Agreed. I have it on my radar.

Thanks,

	tglx