All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
@ 2017-02-11  1:37 ` Jess Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: Thomas Gleixner, Marc Zyngier, open list:IRQ SUBSYSTEM
  Cc: kernel-hardening, Jess Frazelle

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.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 kernel/irq/generic-chip.c | 2 +-
 kernel/irq/msi.c          | 2 +-
 kernel/irq/pm.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index ee32870079c9..cca63dbaabea 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -623,7 +623,7 @@ static void irq_gc_shutdown(void)
 	}
 }

-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,
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index ee230063f033..0e5b723f710f 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -217,7 +217,7 @@ static int msi_domain_ops_check(struct irq_domain *domain,
 	return 0;
 }

-static struct msi_domain_ops msi_domain_ops_default = {
+static struct msi_domain_ops msi_domain_ops_default __ro_after_init = {
 	.get_hwirq	= msi_domain_ops_get_hwirq,
 	.msi_init	= msi_domain_ops_init,
 	.msi_check	= msi_domain_ops_check,
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index cea1de0161f1..d6b889bed323 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -185,7 +185,7 @@ static void irq_pm_syscore_resume(void)
 	resume_irqs(true);
 }

-static struct syscore_ops irq_pm_syscore_ops = {
+static struct syscore_ops irq_pm_syscore_ops __ro_after_init = {
 	.resume		= irq_pm_syscore_resume,
 };

--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [kernel-hardening] [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
@ 2017-02-11  1:37 ` Jess Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: Thomas Gleixner, Marc Zyngier, open list:IRQ SUBSYSTEM
  Cc: kernel-hardening, Jess Frazelle

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.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 kernel/irq/generic-chip.c | 2 +-
 kernel/irq/msi.c          | 2 +-
 kernel/irq/pm.c           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index ee32870079c9..cca63dbaabea 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -623,7 +623,7 @@ static void irq_gc_shutdown(void)
 	}
 }

-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,
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
index ee230063f033..0e5b723f710f 100644
--- a/kernel/irq/msi.c
+++ b/kernel/irq/msi.c
@@ -217,7 +217,7 @@ static int msi_domain_ops_check(struct irq_domain *domain,
 	return 0;
 }

-static struct msi_domain_ops msi_domain_ops_default = {
+static struct msi_domain_ops msi_domain_ops_default __ro_after_init = {
 	.get_hwirq	= msi_domain_ops_get_hwirq,
 	.msi_init	= msi_domain_ops_init,
 	.msi_check	= msi_domain_ops_check,
diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index cea1de0161f1..d6b889bed323 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -185,7 +185,7 @@ static void irq_pm_syscore_resume(void)
 	resume_irqs(true);
 }

-static struct syscore_ops irq_pm_syscore_ops = {
+static struct syscore_ops irq_pm_syscore_ops __ro_after_init = {
 	.resume		= irq_pm_syscore_resume,
 };

--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v2 2/5] time: mark syscore_ops as __ro_after_init
  2017-02-11  1:37 ` [kernel-hardening] " Jess Frazelle
@ 2017-02-11  1:37   ` Jess Frazelle
  -1 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: John Stultz, Thomas Gleixner, Jess Frazelle, Rik van Riel, open list
  Cc: kernel-hardening

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 ops.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
Acked-by: Rik van Riel <riel@redhat.com>
---
 kernel/time/sched_clock.c | 2 +-
 kernel/time/timekeeping.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index a26036d37a38..5df2fc07300b 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -289,7 +289,7 @@ static void sched_clock_resume(void)
 	rd->read_sched_clock = cd.actual_read_sched_clock;
 }

-static struct syscore_ops sched_clock_ops = {
+static struct syscore_ops sched_clock_ops __ro_after_init = {
 	.suspend	= sched_clock_suspend,
 	.resume		= sched_clock_resume,
 };
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index db087d7e106d..467e3021723a 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1756,7 +1756,7 @@ int timekeeping_suspend(void)
 }

 /* sysfs resume/suspend bits for timekeeping */
-static struct syscore_ops timekeeping_syscore_ops = {
+static struct syscore_ops timekeeping_syscore_ops __ro_after_init = {
 	.resume		= timekeeping_resume,
 	.suspend	= timekeeping_suspend,
 };
--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [kernel-hardening] [PATCH v2 2/5] time: mark syscore_ops as __ro_after_init
@ 2017-02-11  1:37   ` Jess Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: John Stultz, Thomas Gleixner, Jess Frazelle, Rik van Riel, open list
  Cc: kernel-hardening

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 ops.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
Acked-by: Rik van Riel <riel@redhat.com>
---
 kernel/time/sched_clock.c | 2 +-
 kernel/time/timekeeping.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index a26036d37a38..5df2fc07300b 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -289,7 +289,7 @@ static void sched_clock_resume(void)
 	rd->read_sched_clock = cd.actual_read_sched_clock;
 }

-static struct syscore_ops sched_clock_ops = {
+static struct syscore_ops sched_clock_ops __ro_after_init = {
 	.suspend	= sched_clock_suspend,
 	.resume		= sched_clock_resume,
 };
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index db087d7e106d..467e3021723a 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1756,7 +1756,7 @@ int timekeeping_suspend(void)
 }

 /* sysfs resume/suspend bits for timekeeping */
-static struct syscore_ops timekeeping_syscore_ops = {
+static struct syscore_ops timekeeping_syscore_ops __ro_after_init = {
 	.resume		= timekeeping_resume,
 	.suspend	= timekeeping_suspend,
 };
--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-11  1:37 ` [kernel-hardening] " Jess Frazelle
@ 2017-02-11  1:37   ` Jess Frazelle
  -1 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Bjorn Helgaas, Keith Busch, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list
  Cc: kernel-hardening, Jess Frazelle

Marked msi_domain_ops structs as __ro_after_init when called only during init.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 drivers/pci/host/pci-hyperv.c | 2 +-
 drivers/pci/host/vmd.c        | 2 +-
 drivers/pci/msi.c             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 3efcc7bdc5fb..f05b93689d8f 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -958,7 +958,7 @@ static irq_hw_number_t hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
 	return arg->msi_hwirq;
 }

-static struct msi_domain_ops hv_msi_ops = {
+static struct msi_domain_ops hv_msi_ops __ro_after_init = {
 	.get_hwirq	= hv_msi_domain_ops_get_hwirq,
 	.msi_prepare	= pci_msi_prepare,
 	.set_desc	= pci_msi_set_desc,
diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
index 18ef1a93c10a..152c461538e4 100644
--- a/drivers/pci/host/vmd.c
+++ b/drivers/pci/host/vmd.c
@@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
 	arg->desc = desc;
 }

-static struct msi_domain_ops vmd_msi_domain_ops = {
+static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= vmd_get_hwirq,
 	.msi_init	= vmd_msi_init,
 	.msi_free	= vmd_msi_free,
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 50c5003295ca..93141d5e2d1c 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1413,7 +1413,7 @@ static void pci_msi_domain_set_desc(msi_alloc_info_t *arg,
 #define pci_msi_domain_set_desc		NULL
 #endif

-static struct msi_domain_ops pci_msi_domain_ops_default = {
+static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init = {
 	.set_desc	= pci_msi_domain_set_desc,
 	.msi_check	= pci_msi_domain_check_cap,
 	.handle_error	= pci_msi_domain_handle_error,
--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [kernel-hardening] [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-11  1:37   ` Jess Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Bjorn Helgaas, Keith Busch, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list
  Cc: kernel-hardening, Jess Frazelle

Marked msi_domain_ops structs as __ro_after_init when called only during init.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 drivers/pci/host/pci-hyperv.c | 2 +-
 drivers/pci/host/vmd.c        | 2 +-
 drivers/pci/msi.c             | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
index 3efcc7bdc5fb..f05b93689d8f 100644
--- a/drivers/pci/host/pci-hyperv.c
+++ b/drivers/pci/host/pci-hyperv.c
@@ -958,7 +958,7 @@ static irq_hw_number_t hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
 	return arg->msi_hwirq;
 }

-static struct msi_domain_ops hv_msi_ops = {
+static struct msi_domain_ops hv_msi_ops __ro_after_init = {
 	.get_hwirq	= hv_msi_domain_ops_get_hwirq,
 	.msi_prepare	= pci_msi_prepare,
 	.set_desc	= pci_msi_set_desc,
diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
index 18ef1a93c10a..152c461538e4 100644
--- a/drivers/pci/host/vmd.c
+++ b/drivers/pci/host/vmd.c
@@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
 	arg->desc = desc;
 }

-static struct msi_domain_ops vmd_msi_domain_ops = {
+static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= vmd_get_hwirq,
 	.msi_init	= vmd_msi_init,
 	.msi_free	= vmd_msi_free,
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 50c5003295ca..93141d5e2d1c 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -1413,7 +1413,7 @@ static void pci_msi_domain_set_desc(msi_alloc_info_t *arg,
 #define pci_msi_domain_set_desc		NULL
 #endif

-static struct msi_domain_ops pci_msi_domain_ops_default = {
+static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init = {
 	.set_desc	= pci_msi_domain_set_desc,
 	.msi_check	= pci_msi_domain_check_cap,
 	.handle_error	= pci_msi_domain_handle_error,
--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v2 4/5] staging: set msi_domain_ops as __ro_after_init
  2017-02-11  1:37 ` [kernel-hardening] " Jess Frazelle
@ 2017-02-11  1:37   ` Jess Frazelle
  -1 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: Stuart Yoder, Greg Kroah-Hartman, Bharat Bhushan,
	Bhaktipriya Shridhar, Nipun Gupta, Jess Frazelle,
	open list:QORIQ DPAA2 FSL-MC BUS DRIVER,
	open list:STAGING SUBSYSTEM
  Cc: kernel-hardening

Marked msi_domain_ops structs as __ro_after_init when called only during init.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
index 6b1cd574644f..0e2c1b5e13b7 100644
--- a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
+++ b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
@@ -51,7 +51,7 @@ static int its_fsl_mc_msi_prepare(struct irq_domain *msi_domain,
 	return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);
 }

-static struct msi_domain_ops its_fsl_mc_msi_ops = {
+static struct msi_domain_ops its_fsl_mc_msi_ops __ro_after_init = {
 	.msi_prepare = its_fsl_mc_msi_prepare,
 };

--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [kernel-hardening] [PATCH v2 4/5] staging: set msi_domain_ops as __ro_after_init
@ 2017-02-11  1:37   ` Jess Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: Stuart Yoder, Greg Kroah-Hartman, Bharat Bhushan,
	Bhaktipriya Shridhar, Nipun Gupta, Jess Frazelle,
	open list:QORIQ DPAA2 FSL-MC BUS DRIVER,
	open list:STAGING SUBSYSTEM
  Cc: kernel-hardening

Marked msi_domain_ops structs as __ro_after_init when called only during init.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
index 6b1cd574644f..0e2c1b5e13b7 100644
--- a/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
+++ b/drivers/staging/fsl-mc/bus/irq-gic-v3-its-fsl-mc-msi.c
@@ -51,7 +51,7 @@ static int its_fsl_mc_msi_prepare(struct irq_domain *msi_domain,
 	return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);
 }

-static struct msi_domain_ops its_fsl_mc_msi_ops = {
+static struct msi_domain_ops its_fsl_mc_msi_ops __ro_after_init = {
 	.msi_prepare = its_fsl_mc_msi_prepare,
 };

--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [PATCH v2 5/5] x86: set msi_domain_ops as __ro_after_init
  2017-02-11  1:37 ` [kernel-hardening] " Jess Frazelle
@ 2017-02-11  1:37   ` Jess Frazelle
  -1 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
	Jess Frazelle, Kees Cook,
	open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)
  Cc: kernel-hardening

Marked msi_domain_ops structs as __ro_after_init when called only during init.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 arch/x86/kernel/apic/msi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
index 015bbf30e3e3..27783a1e7166 100644
--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -121,7 +121,7 @@ void pci_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
 }
 EXPORT_SYMBOL_GPL(pci_msi_set_desc);

-static struct msi_domain_ops pci_msi_domain_ops = {
+static struct msi_domain_ops pci_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= pci_msi_get_hwirq,
 	.msi_prepare	= pci_msi_prepare,
 	.set_desc	= pci_msi_set_desc,
@@ -207,7 +207,7 @@ static int dmar_msi_init(struct irq_domain *domain,
 	return 0;
 }

-static struct msi_domain_ops dmar_msi_domain_ops = {
+static struct msi_domain_ops dmar_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= dmar_msi_get_hwirq,
 	.msi_init	= dmar_msi_init,
 };
@@ -304,7 +304,7 @@ static void hpet_msi_free(struct irq_domain *domain,
 	irq_clear_status_flags(virq, IRQ_MOVE_PCNTXT);
 }

-static struct msi_domain_ops hpet_msi_domain_ops = {
+static struct msi_domain_ops hpet_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= hpet_msi_get_hwirq,
 	.msi_init	= hpet_msi_init,
 	.msi_free	= hpet_msi_free,
--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* [kernel-hardening] [PATCH v2 5/5] x86: set msi_domain_ops as __ro_after_init
@ 2017-02-11  1:37   ` Jess Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11  1:37 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	maintainer:X86 ARCHITECTURE 32-BIT AND 64-BIT, Jess Frazelle,
	Kees Cook, open list:X86 ARCHITECTURE 32-BIT AND 64-BIT
  Cc: kernel-hardening

Marked msi_domain_ops structs as __ro_after_init when called only during init.
This protects the data structure from accidental corruption.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.com>
---
 arch/x86/kernel/apic/msi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/apic/msi.c b/arch/x86/kernel/apic/msi.c
index 015bbf30e3e3..27783a1e7166 100644
--- a/arch/x86/kernel/apic/msi.c
+++ b/arch/x86/kernel/apic/msi.c
@@ -121,7 +121,7 @@ void pci_msi_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
 }
 EXPORT_SYMBOL_GPL(pci_msi_set_desc);

-static struct msi_domain_ops pci_msi_domain_ops = {
+static struct msi_domain_ops pci_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= pci_msi_get_hwirq,
 	.msi_prepare	= pci_msi_prepare,
 	.set_desc	= pci_msi_set_desc,
@@ -207,7 +207,7 @@ static int dmar_msi_init(struct irq_domain *domain,
 	return 0;
 }

-static struct msi_domain_ops dmar_msi_domain_ops = {
+static struct msi_domain_ops dmar_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= dmar_msi_get_hwirq,
 	.msi_init	= dmar_msi_init,
 };
@@ -304,7 +304,7 @@ static void hpet_msi_free(struct irq_domain *domain,
 	irq_clear_status_flags(virq, IRQ_MOVE_PCNTXT);
 }

-static struct msi_domain_ops hpet_msi_domain_ops = {
+static struct msi_domain_ops hpet_msi_domain_ops __ro_after_init = {
 	.get_hwirq	= hpet_msi_get_hwirq,
 	.msi_init	= hpet_msi_init,
 	.msi_free	= hpet_msi_free,
--
2.11.0

^ permalink raw reply related	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 2/5] time: mark syscore_ops as __ro_after_init
  2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
@ 2017-02-11  2:12     ` John Stultz
  -1 siblings, 0 replies; 46+ messages in thread
From: John Stultz @ 2017-02-11  2:12 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Thomas Gleixner, Rik van Riel, open list, kernel-hardening

On Fri, Feb 10, 2017 at 5:37 PM, Jess Frazelle <me@jessfraz.com> wrote:
> 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 ops.
> This protects the data structure from accidental corruption.
>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>
> Acked-by: Rik van Riel <riel@redhat.com>

Thanks for sending this out. Looks reasonable to me. I'll queue it for
testing, targeting for 4.12.

thanks
-john

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 2/5] time: mark syscore_ops as __ro_after_init
@ 2017-02-11  2:12     ` John Stultz
  0 siblings, 0 replies; 46+ messages in thread
From: John Stultz @ 2017-02-11  2:12 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Thomas Gleixner, Rik van Riel, open list, kernel-hardening

On Fri, Feb 10, 2017 at 5:37 PM, Jess Frazelle <me@jessfraz.com> wrote:
> 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 ops.
> This protects the data structure from accidental corruption.
>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>
> Acked-by: Rik van Riel <riel@redhat.com>

Thanks for sending this out. Looks reasonable to me. I'll queue it for
testing, targeting for 4.12.

thanks
-john

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
  2017-02-11  1:37 ` [kernel-hardening] " Jess Frazelle
@ 2017-02-11  9:14   ` Thomas Gleixner
  -1 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11  9:14 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
@ 2017-02-11  9:14   ` Thomas Gleixner
  0 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11  9:14 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
  2017-02-11  9:14   ` [kernel-hardening] " Thomas Gleixner
@ 2017-02-11  9:23     ` Thomas Gleixner
  -1 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11  9:23 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening



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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
@ 2017-02-11  9:23     ` Thomas Gleixner
  0 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11  9:23 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening



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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 2/5] time: mark syscore_ops as __ro_after_init
  2017-02-11  2:12     ` [kernel-hardening] " John Stultz
@ 2017-02-11  9:23       ` Thomas Gleixner
  -1 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11  9:23 UTC (permalink / raw)
  To: John Stultz; +Cc: Jess Frazelle, Rik van Riel, open list, kernel-hardening

On Fri, 10 Feb 2017, John Stultz wrote:

> On Fri, Feb 10, 2017 at 5:37 PM, Jess Frazelle <me@jessfraz.com> wrote:
> > 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 ops.
> > This protects the data structure from accidental corruption.
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Jess Frazelle <me@jessfraz.com>
> > Acked-by: Rik van Riel <riel@redhat.com>
> 
> Thanks for sending this out. Looks reasonable to me. I'll queue it for
> testing, targeting for 4.12.

NAK. See:

  http://lkml.kernel.org/r/alpine.DEB.2.20.1702110948030.3734@nanos

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 2/5] time: mark syscore_ops as __ro_after_init
@ 2017-02-11  9:23       ` Thomas Gleixner
  0 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11  9:23 UTC (permalink / raw)
  To: John Stultz; +Cc: Jess Frazelle, Rik van Riel, open list, kernel-hardening

On Fri, 10 Feb 2017, John Stultz wrote:

> On Fri, Feb 10, 2017 at 5:37 PM, Jess Frazelle <me@jessfraz.com> wrote:
> > 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 ops.
> > This protects the data structure from accidental corruption.
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Jess Frazelle <me@jessfraz.com>
> > Acked-by: Rik van Riel <riel@redhat.com>
> 
> Thanks for sending this out. Looks reasonable to me. I'll queue it for
> testing, targeting for 4.12.

NAK. See:

  http://lkml.kernel.org/r/alpine.DEB.2.20.1702110948030.3734@nanos

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
  2017-02-11  9:14   ` [kernel-hardening] " Thomas Gleixner
@ 2017-02-11 10:48     ` Jess Frazelle
  -1 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11 10:48 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening



On February 11, 2017 1:14:52 AM PST, Thomas Gleixner <tglx@linutronix.de> 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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
@ 2017-02-11 10:48     ` Jess Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jess Frazelle @ 2017-02-11 10:48 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening



On February 11, 2017 1:14:52 AM PST, Thomas Gleixner <tglx@linutronix.de> 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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
  2017-02-11 10:48     ` [kernel-hardening] " Jess Frazelle
@ 2017-02-11 12:00       ` Thomas Gleixner
  -1 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11 12:00 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening

On Sat, 11 Feb 2017, Jess Frazelle wrote:
> On February 11, 2017 1:14:52 AM PST, Thomas Gleixner <tglx@linutronix.de> wrote:
> >The same is true for cpuhotunplug operations.
> 
> This makes sense. Will remove.

That's true for all other patches touching sysops as well. But instead of
giving up I'd recommend to look into the following:

Go through all callsites which use un/register_syscore_ops() and figure out
how many of them are possibly called post init. From a quick grep I can
only find the KVM module, but there might be more.

Lets assume it's KVM only. So you could do the following:

Put something like this into virt/kvm/kvm_main.c, which is a builtin file

static struct syscore_ops ops __ro_after_init = {
       ....
};

int __init foo()
{
    register_ops(&ops);
}

and because we know that kvm is single instance you can just have:

static struct syscore_ops *kvm_ops;

void kvm_set_sysop(*vmx_ops)
{
	kvm_ops = ops;
}

and then have the kvm_syscore callbacks:

static callback()
{
	if (kvm_ops)
	   kvm_ops->callback()
}

Sanity checks and serialization omitted. Then switch kvm_exit/init over to
it.

After that you can make all syscore_ops __ro_after_init, remove the export
from (un)register_syscore_ops() and make that __init.

Not much of an effort and probably worth the trouble.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
@ 2017-02-11 12:00       ` Thomas Gleixner
  0 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-11 12:00 UTC (permalink / raw)
  To: Jess Frazelle; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, kernel-hardening

On Sat, 11 Feb 2017, Jess Frazelle wrote:
> On February 11, 2017 1:14:52 AM PST, Thomas Gleixner <tglx@linutronix.de> wrote:
> >The same is true for cpuhotunplug operations.
> 
> This makes sense. Will remove.

That's true for all other patches touching sysops as well. But instead of
giving up I'd recommend to look into the following:

Go through all callsites which use un/register_syscore_ops() and figure out
how many of them are possibly called post init. From a quick grep I can
only find the KVM module, but there might be more.

Lets assume it's KVM only. So you could do the following:

Put something like this into virt/kvm/kvm_main.c, which is a builtin file

static struct syscore_ops ops __ro_after_init = {
       ....
};

int __init foo()
{
    register_ops(&ops);
}

and because we know that kvm is single instance you can just have:

static struct syscore_ops *kvm_ops;

void kvm_set_sysop(*vmx_ops)
{
	kvm_ops = ops;
}

and then have the kvm_syscore callbacks:

static callback()
{
	if (kvm_ops)
	   kvm_ops->callback()
}

Sanity checks and serialization omitted. Then switch kvm_exit/init over to
it.

After that you can make all syscore_ops __ro_after_init, remove the export
from (un)register_syscore_ops() and make that __init.

Not much of an effort and probably worth the trouble.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
  2017-02-11 12:00       ` [kernel-hardening] " Thomas Gleixner
@ 2017-02-11 12:17         ` Jessica Frazelle
  -1 siblings, 0 replies; 46+ messages in thread
From: Jessica Frazelle @ 2017-02-11 12:17 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, Kernel Hardening

Thanks, I'll check them out.

On Sat, Feb 11, 2017 at 4:00 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sat, 11 Feb 2017, Jess Frazelle wrote:
> > On February 11, 2017 1:14:52 AM PST, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >The same is true for cpuhotunplug operations.
> >
> > This makes sense. Will remove.
>
> That's true for all other patches touching sysops as well. But instead of
> giving up I'd recommend to look into the following:
>
> Go through all callsites which use un/register_syscore_ops() and figure out
> how many of them are possibly called post init. From a quick grep I can
> only find the KVM module, but there might be more.
>
> Lets assume it's KVM only. So you could do the following:
>
> Put something like this into virt/kvm/kvm_main.c, which is a builtin file
>
> static struct syscore_ops ops __ro_after_init = {
>        ....
> };
>
> int __init foo()
> {
>     register_ops(&ops);
> }
>
> and because we know that kvm is single instance you can just have:
>
> static struct syscore_ops *kvm_ops;
>
> void kvm_set_sysop(*vmx_ops)
> {
>         kvm_ops = ops;
> }
>
> and then have the kvm_syscore callbacks:
>
> static callback()
> {
>         if (kvm_ops)
>            kvm_ops->callback()
> }
>
> Sanity checks and serialization omitted. Then switch kvm_exit/init over to
> it.
>
> After that you can make all syscore_ops __ro_after_init, remove the export
> from (un)register_syscore_ops() and make that __init.
>
> Not much of an effort and probably worth the trouble.
>
> Thanks,
>
>         tglx
>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init
@ 2017-02-11 12:17         ` Jessica Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jessica Frazelle @ 2017-02-11 12:17 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Marc Zyngier, open list:IRQ SUBSYSTEM, Kernel Hardening

Thanks, I'll check them out.

On Sat, Feb 11, 2017 at 4:00 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Sat, 11 Feb 2017, Jess Frazelle wrote:
> > On February 11, 2017 1:14:52 AM PST, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >The same is true for cpuhotunplug operations.
> >
> > This makes sense. Will remove.
>
> That's true for all other patches touching sysops as well. But instead of
> giving up I'd recommend to look into the following:
>
> Go through all callsites which use un/register_syscore_ops() and figure out
> how many of them are possibly called post init. From a quick grep I can
> only find the KVM module, but there might be more.
>
> Lets assume it's KVM only. So you could do the following:
>
> Put something like this into virt/kvm/kvm_main.c, which is a builtin file
>
> static struct syscore_ops ops __ro_after_init = {
>        ....
> };
>
> int __init foo()
> {
>     register_ops(&ops);
> }
>
> and because we know that kvm is single instance you can just have:
>
> static struct syscore_ops *kvm_ops;
>
> void kvm_set_sysop(*vmx_ops)
> {
>         kvm_ops = ops;
> }
>
> and then have the kvm_syscore callbacks:
>
> static callback()
> {
>         if (kvm_ops)
>            kvm_ops->callback()
> }
>
> Sanity checks and serialization omitted. Then switch kvm_exit/init over to
> it.
>
> After that you can make all syscore_ops __ro_after_init, remove the export
> from (un)register_syscore_ops() and make that __init.
>
> Not much of an effort and probably worth the trouble.
>
> Thanks,
>
>         tglx
>

^ permalink raw reply	[flat|nested] 46+ messages in thread

* RE: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
  (?)
@ 2017-02-12  4:08     ` KY Srinivasan
  -1 siblings, 0 replies; 46+ messages in thread
From: KY Srinivasan @ 2017-02-12  4:08 UTC (permalink / raw)
  To: Jess Frazelle, Haiyang Zhang, Stephen Hemminger, Bjorn Helgaas,
	Keith Busch, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list
  Cc: kernel-hardening



> -----Original Message-----
> From: Jess Frazelle [mailto:me@jessfraz.com]
> Sent: Friday, February 10, 2017 5:38 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>; Keith
> Busch <keith.busch@intel.com>; open list:Hyper-V CORE AND DRIVERS
> <devel@linuxdriverproject.org>; open list:PCI SUBSYSTEM <linux-
> pci@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Cc: kernel-hardening@lists.openwall.com; Jess Frazelle <me@jessfraz.com>
> Subject: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
> 
> Marked msi_domain_ops structs as __ro_after_init when called only during
> init.
> This protects the data structure from accidental corruption.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>

Acked-by: K. Y. Srinivasan <kys@microsoft.com>

K. Y

> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c        | 2 +-
>  drivers/pci/msi.c             | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 3efcc7bdc5fb..f05b93689d8f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -958,7 +958,7 @@ static irq_hw_number_t
> hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
>  	return arg->msi_hwirq;
>  }
> 
> -static struct msi_domain_ops hv_msi_ops = {
> +static struct msi_domain_ops hv_msi_ops __ro_after_init = {
>  	.get_hwirq	= hv_msi_domain_ops_get_hwirq,
>  	.msi_prepare	= pci_msi_prepare,
>  	.set_desc	= pci_msi_set_desc,
> diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
> index 18ef1a93c10a..152c461538e4 100644
> --- a/drivers/pci/host/vmd.c
> +++ b/drivers/pci/host/vmd.c
> @@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
>  	arg->desc = desc;
>  }
> 
> -static struct msi_domain_ops vmd_msi_domain_ops = {
> +static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init = {
>  	.get_hwirq	= vmd_get_hwirq,
>  	.msi_init	= vmd_msi_init,
>  	.msi_free	= vmd_msi_free,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 50c5003295ca..93141d5e2d1c 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1413,7 +1413,7 @@ static void
> pci_msi_domain_set_desc(msi_alloc_info_t *arg,
>  #define pci_msi_domain_set_desc		NULL
>  #endif
> 
> -static struct msi_domain_ops pci_msi_domain_ops_default = {
> +static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init
> = {
>  	.set_desc	= pci_msi_domain_set_desc,
>  	.msi_check	= pci_msi_domain_check_cap,
>  	.handle_error	= pci_msi_domain_handle_error,
> --
> 2.11.0

^ permalink raw reply	[flat|nested] 46+ messages in thread

* RE: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-12  4:08     ` KY Srinivasan
  0 siblings, 0 replies; 46+ messages in thread
From: KY Srinivasan @ 2017-02-12  4:08 UTC (permalink / raw)
  To: Jess Frazelle, Haiyang Zhang, Stephen Hemminger, Bjorn Helgaas,
	Keith Busch, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list
  Cc: kernel-hardening



> -----Original Message-----
> From: Jess Frazelle [mailto:me@jessfraz.com]
> Sent: Friday, February 10, 2017 5:38 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>; Keith
> Busch <keith.busch@intel.com>; open list:Hyper-V CORE AND DRIVERS
> <devel@linuxdriverproject.org>; open list:PCI SUBSYSTEM <linux-
> pci@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Cc: kernel-hardening@lists.openwall.com; Jess Frazelle <me@jessfraz.com>
> Subject: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
>=20
> Marked msi_domain_ops structs as __ro_after_init when called only during
> init.
> This protects the data structure from accidental corruption.
>=20
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>

Acked-by: K. Y. Srinivasan <kys@microsoft.com>

K. Y

> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c        | 2 +-
>  drivers/pci/msi.c             | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>=20
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.=
c
> index 3efcc7bdc5fb..f05b93689d8f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -958,7 +958,7 @@ static irq_hw_number_t
> hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
>  	return arg->msi_hwirq;
>  }
>=20
> -static struct msi_domain_ops hv_msi_ops =3D {
> +static struct msi_domain_ops hv_msi_ops __ro_after_init =3D {
>  	.get_hwirq	=3D hv_msi_domain_ops_get_hwirq,
>  	.msi_prepare	=3D pci_msi_prepare,
>  	.set_desc	=3D pci_msi_set_desc,
> diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
> index 18ef1a93c10a..152c461538e4 100644
> --- a/drivers/pci/host/vmd.c
> +++ b/drivers/pci/host/vmd.c
> @@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
>  	arg->desc =3D desc;
>  }
>=20
> -static struct msi_domain_ops vmd_msi_domain_ops =3D {
> +static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init =3D {
>  	.get_hwirq	=3D vmd_get_hwirq,
>  	.msi_init	=3D vmd_msi_init,
>  	.msi_free	=3D vmd_msi_free,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 50c5003295ca..93141d5e2d1c 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1413,7 +1413,7 @@ static void
> pci_msi_domain_set_desc(msi_alloc_info_t *arg,
>  #define pci_msi_domain_set_desc		NULL
>  #endif
>=20
> -static struct msi_domain_ops pci_msi_domain_ops_default =3D {
> +static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init
> =3D {
>  	.set_desc	=3D pci_msi_domain_set_desc,
>  	.msi_check	=3D pci_msi_domain_check_cap,
>  	.handle_error	=3D pci_msi_domain_handle_error,
> --
> 2.11.0

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] RE: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-12  4:08     ` KY Srinivasan
  0 siblings, 0 replies; 46+ messages in thread
From: KY Srinivasan @ 2017-02-12  4:08 UTC (permalink / raw)
  To: Jess Frazelle, Haiyang Zhang, Stephen Hemminger, Bjorn Helgaas,
	Keith Busch, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list
  Cc: kernel-hardening



> -----Original Message-----
> From: Jess Frazelle [mailto:me@jessfraz.com]
> Sent: Friday, February 10, 2017 5:38 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; Bjorn Helgaas <bhelgaas@google.com>; Keith
> Busch <keith.busch@intel.com>; open list:Hyper-V CORE AND DRIVERS
> <devel@linuxdriverproject.org>; open list:PCI SUBSYSTEM <linux-
> pci@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>
> Cc: kernel-hardening@lists.openwall.com; Jess Frazelle <me@jessfraz.com>
> Subject: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
> 
> Marked msi_domain_ops structs as __ro_after_init when called only during
> init.
> This protects the data structure from accidental corruption.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>

Acked-by: K. Y. Srinivasan <kys@microsoft.com>

K. Y

> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c        | 2 +-
>  drivers/pci/msi.c             | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 3efcc7bdc5fb..f05b93689d8f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -958,7 +958,7 @@ static irq_hw_number_t
> hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
>  	return arg->msi_hwirq;
>  }
> 
> -static struct msi_domain_ops hv_msi_ops = {
> +static struct msi_domain_ops hv_msi_ops __ro_after_init = {
>  	.get_hwirq	= hv_msi_domain_ops_get_hwirq,
>  	.msi_prepare	= pci_msi_prepare,
>  	.set_desc	= pci_msi_set_desc,
> diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
> index 18ef1a93c10a..152c461538e4 100644
> --- a/drivers/pci/host/vmd.c
> +++ b/drivers/pci/host/vmd.c
> @@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg,
> struct msi_desc *desc)
>  	arg->desc = desc;
>  }
> 
> -static struct msi_domain_ops vmd_msi_domain_ops = {
> +static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init = {
>  	.get_hwirq	= vmd_get_hwirq,
>  	.msi_init	= vmd_msi_init,
>  	.msi_free	= vmd_msi_free,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 50c5003295ca..93141d5e2d1c 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1413,7 +1413,7 @@ static void
> pci_msi_domain_set_desc(msi_alloc_info_t *arg,
>  #define pci_msi_domain_set_desc		NULL
>  #endif
> 
> -static struct msi_domain_ops pci_msi_domain_ops_default = {
> +static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init
> = {
>  	.set_desc	= pci_msi_domain_set_desc,
>  	.msi_check	= pci_msi_domain_check_cap,
>  	.handle_error	= pci_msi_domain_handle_error,
> --
> 2.11.0

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
@ 2017-02-13 18:14     ` Keith Busch
  -1 siblings, 0 replies; 46+ messages in thread
From: Keith Busch @ 2017-02-13 18:14 UTC (permalink / raw)
  To: Jess Frazelle
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Bjorn Helgaas, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list, kernel-hardening

On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
> Marked msi_domain_ops structs as __ro_after_init when called only during init.
> This protects the data structure from accidental corruption.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>
>
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c        | 2 +-

Ok for vmd driver.

Acked-by: Keith Busch <keith.busch@intel.com>

>  drivers/pci/msi.c             | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-13 18:14     ` Keith Busch
  0 siblings, 0 replies; 46+ messages in thread
From: Keith Busch @ 2017-02-13 18:14 UTC (permalink / raw)
  To: Jess Frazelle
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Bjorn Helgaas, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list, kernel-hardening

On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
> Marked msi_domain_ops structs as __ro_after_init when called only during init.
> This protects the data structure from accidental corruption.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>
>
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c        | 2 +-

Ok for vmd driver.

Acked-by: Keith Busch <keith.busch@intel.com>

>  drivers/pci/msi.c             | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
@ 2017-02-15 20:33     ` Bjorn Helgaas
  -1 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-02-15 20:33 UTC (permalink / raw)
  To: Jess Frazelle
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Bjorn Helgaas, Keith Busch, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list, kernel-hardening, Kees Cook,
	Thomas Gleixner, Marc Zyngier

[+cc Kees, Thomas, Marc]

Hi Jess,

Thanks for the patch!

On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
> Marked msi_domain_ops structs as __ro_after_init when called only during init.
> This protects the data structure from accidental corruption.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>
> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c        | 2 +-
>  drivers/pci/msi.c             | 2 +-

I understand the value of __ro_after_init, but I'm not certain about
sprinkling it around in seemingly random places because it's hard to
know where to put it and whether we put it in all the right places.

How did you choose these three files to change?  There are other
instances of struct msi_domain_ops that use MSI_FLAG_USE_DEF_DOM_OPS.
Should they be changed, too?  If not, is there a rule to figure out
which ones should be made __ro_after_init?

I wonder if adding __ro_after_init is really the best solution.  I
looked at VMD to see how vmd_msi_domain_ops is updated.  Here are the
definitions:

  static struct msi_domain_ops vmd_msi_domain_ops = {
    .get_hwirq = vmd_get_hwirq,
    .msi_init  = vmd_msi_init,
    ...
  };

  static struct msi_domain_info vmd_msi_domain_info = {
    .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
             MSI_FLAG_PCI_MSIX,
    .ops = &vmd_msi_domain_ops,
    ...
  };

Both vmd_msi_domain_ops and vmd_msi_domain_info are statically
initialized, but not completely.  Then we pass a pointer to
pci_msi_create_irq_domain(), which fills in defaults for some of the
function pointers that weren't statically initialized:

  vmd_enable_domain()
    pci_msi_create_irq_domain(NULL, &vmd_msi_domain_info, ...)
      if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
        pci_msi_domain_update_dom_ops(info)
	  if (ops->set_desc == NULL)
	    ops->msi_check = pci_msi_domain_check_cap

We know at build-time what all the function pointers will be, so in
principle we should be able to make the struct const, which would be
even better than __ro_after_init.

For example, we could require that callers set every function pointer
before calling pci_msi_create_irq_domain(), using the default ones
(pci_msi_domain_set_desc, pci_msi_domain_check_cap,
pci_msi_domain_handle_error) if it doesn't need to override them,
e.g.,

  static struct msi_domain_ops vmd_msi_domain_ops = {
    .get_hwirq = vmd_get_hwirq,
    .msi_check = pci_msi_domain_check_cap,
  };

Or we could leave NULL pointers in the structure and have the code
that calls through the function pointers check for NULL and call the
default itself, e.g.,

  if (ops->msi_check)
    ops->msi_check(...)
  else
    pci_msi_domain_check_cap(...)

It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
the commits below.  I would CC: him for his thoughts, but I don't
have a current email address.

  aeeb59657c35 ("genirq: Provide default callbacks for msi_domain_ops")
  3878eaefb89a ("PCI/MSI: Enhance core to support hierarchy irqdomain")

Bjorn

>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 3efcc7bdc5fb..f05b93689d8f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -958,7 +958,7 @@ static irq_hw_number_t hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
>  	return arg->msi_hwirq;
>  }
> 
> -static struct msi_domain_ops hv_msi_ops = {
> +static struct msi_domain_ops hv_msi_ops __ro_after_init = {
>  	.get_hwirq	= hv_msi_domain_ops_get_hwirq,
>  	.msi_prepare	= pci_msi_prepare,
>  	.set_desc	= pci_msi_set_desc,
> diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
> index 18ef1a93c10a..152c461538e4 100644
> --- a/drivers/pci/host/vmd.c
> +++ b/drivers/pci/host/vmd.c
> @@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
>  	arg->desc = desc;
>  }
> 
> -static struct msi_domain_ops vmd_msi_domain_ops = {
> +static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init = {
>  	.get_hwirq	= vmd_get_hwirq,
>  	.msi_init	= vmd_msi_init,
>  	.msi_free	= vmd_msi_free,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 50c5003295ca..93141d5e2d1c 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1413,7 +1413,7 @@ static void pci_msi_domain_set_desc(msi_alloc_info_t *arg,
>  #define pci_msi_domain_set_desc		NULL
>  #endif
> 
> -static struct msi_domain_ops pci_msi_domain_ops_default = {
> +static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init = {
>  	.set_desc	= pci_msi_domain_set_desc,
>  	.msi_check	= pci_msi_domain_check_cap,
>  	.handle_error	= pci_msi_domain_handle_error,
> --
> 2.11.0
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-15 20:33     ` Bjorn Helgaas
  0 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-02-15 20:33 UTC (permalink / raw)
  To: Jess Frazelle
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Bjorn Helgaas, Keith Busch, open list:Hyper-V CORE AND DRIVERS,
	open list:PCI SUBSYSTEM, open list, kernel-hardening, Kees Cook,
	Thomas Gleixner, Marc Zyngier

[+cc Kees, Thomas, Marc]

Hi Jess,

Thanks for the patch!

On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
> Marked msi_domain_ops structs as __ro_after_init when called only during init.
> This protects the data structure from accidental corruption.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>
> ---
>  drivers/pci/host/pci-hyperv.c | 2 +-
>  drivers/pci/host/vmd.c        | 2 +-
>  drivers/pci/msi.c             | 2 +-

I understand the value of __ro_after_init, but I'm not certain about
sprinkling it around in seemingly random places because it's hard to
know where to put it and whether we put it in all the right places.

How did you choose these three files to change?  There are other
instances of struct msi_domain_ops that use MSI_FLAG_USE_DEF_DOM_OPS.
Should they be changed, too?  If not, is there a rule to figure out
which ones should be made __ro_after_init?

I wonder if adding __ro_after_init is really the best solution.  I
looked at VMD to see how vmd_msi_domain_ops is updated.  Here are the
definitions:

  static struct msi_domain_ops vmd_msi_domain_ops = {
    .get_hwirq = vmd_get_hwirq,
    .msi_init  = vmd_msi_init,
    ...
  };

  static struct msi_domain_info vmd_msi_domain_info = {
    .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
             MSI_FLAG_PCI_MSIX,
    .ops = &vmd_msi_domain_ops,
    ...
  };

Both vmd_msi_domain_ops and vmd_msi_domain_info are statically
initialized, but not completely.  Then we pass a pointer to
pci_msi_create_irq_domain(), which fills in defaults for some of the
function pointers that weren't statically initialized:

  vmd_enable_domain()
    pci_msi_create_irq_domain(NULL, &vmd_msi_domain_info, ...)
      if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
        pci_msi_domain_update_dom_ops(info)
	  if (ops->set_desc == NULL)
	    ops->msi_check = pci_msi_domain_check_cap

We know at build-time what all the function pointers will be, so in
principle we should be able to make the struct const, which would be
even better than __ro_after_init.

For example, we could require that callers set every function pointer
before calling pci_msi_create_irq_domain(), using the default ones
(pci_msi_domain_set_desc, pci_msi_domain_check_cap,
pci_msi_domain_handle_error) if it doesn't need to override them,
e.g.,

  static struct msi_domain_ops vmd_msi_domain_ops = {
    .get_hwirq = vmd_get_hwirq,
    .msi_check = pci_msi_domain_check_cap,
  };

Or we could leave NULL pointers in the structure and have the code
that calls through the function pointers check for NULL and call the
default itself, e.g.,

  if (ops->msi_check)
    ops->msi_check(...)
  else
    pci_msi_domain_check_cap(...)

It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
the commits below.  I would CC: him for his thoughts, but I don't
have a current email address.

  aeeb59657c35 ("genirq: Provide default callbacks for msi_domain_ops")
  3878eaefb89a ("PCI/MSI: Enhance core to support hierarchy irqdomain")

Bjorn

>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-hyperv.c b/drivers/pci/host/pci-hyperv.c
> index 3efcc7bdc5fb..f05b93689d8f 100644
> --- a/drivers/pci/host/pci-hyperv.c
> +++ b/drivers/pci/host/pci-hyperv.c
> @@ -958,7 +958,7 @@ static irq_hw_number_t hv_msi_domain_ops_get_hwirq(struct msi_domain_info *info,
>  	return arg->msi_hwirq;
>  }
> 
> -static struct msi_domain_ops hv_msi_ops = {
> +static struct msi_domain_ops hv_msi_ops __ro_after_init = {
>  	.get_hwirq	= hv_msi_domain_ops_get_hwirq,
>  	.msi_prepare	= pci_msi_prepare,
>  	.set_desc	= pci_msi_set_desc,
> diff --git a/drivers/pci/host/vmd.c b/drivers/pci/host/vmd.c
> index 18ef1a93c10a..152c461538e4 100644
> --- a/drivers/pci/host/vmd.c
> +++ b/drivers/pci/host/vmd.c
> @@ -253,7 +253,7 @@ static void vmd_set_desc(msi_alloc_info_t *arg, struct msi_desc *desc)
>  	arg->desc = desc;
>  }
> 
> -static struct msi_domain_ops vmd_msi_domain_ops = {
> +static struct msi_domain_ops vmd_msi_domain_ops __ro_after_init = {
>  	.get_hwirq	= vmd_get_hwirq,
>  	.msi_init	= vmd_msi_init,
>  	.msi_free	= vmd_msi_free,
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 50c5003295ca..93141d5e2d1c 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -1413,7 +1413,7 @@ static void pci_msi_domain_set_desc(msi_alloc_info_t *arg,
>  #define pci_msi_domain_set_desc		NULL
>  #endif
> 
> -static struct msi_domain_ops pci_msi_domain_ops_default = {
> +static struct msi_domain_ops pci_msi_domain_ops_default __ro_after_init = {
>  	.set_desc	= pci_msi_domain_set_desc,
>  	.msi_check	= pci_msi_domain_check_cap,
>  	.handle_error	= pci_msi_domain_handle_error,
> --
> 2.11.0
> 

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-15 20:33     ` [kernel-hardening] " Bjorn Helgaas
  (?)
@ 2017-02-15 20:46       ` Kees Cook
  -1 siblings, 0 replies; 46+ messages in thread
From: Kees Cook @ 2017-02-15 20:46 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Thomas Gleixner, Marc Zyngier

On Wed, Feb 15, 2017 at 12:33 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Kees, Thomas, Marc]
>
> Hi Jess,
>
> Thanks for the patch!
>
> On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
>> Marked msi_domain_ops structs as __ro_after_init when called only during init.
>> This protects the data structure from accidental corruption.
>>
>> Suggested-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Jess Frazelle <me@jessfraz.com>
>> ---
>>  drivers/pci/host/pci-hyperv.c | 2 +-
>>  drivers/pci/host/vmd.c        | 2 +-
>>  drivers/pci/msi.c             | 2 +-
>
> I understand the value of __ro_after_init, but I'm not certain about
> sprinkling it around in seemingly random places because it's hard to
> know where to put it and whether we put it in all the right places.
>
> How did you choose these three files to change?  There are other
> instances of struct msi_domain_ops that use MSI_FLAG_USE_DEF_DOM_OPS.
> Should they be changed, too?  If not, is there a rule to figure out
> which ones should be made __ro_after_init?
>
> I wonder if adding __ro_after_init is really the best solution.  I
> looked at VMD to see how vmd_msi_domain_ops is updated.  Here are the
> definitions:
>
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_init  = vmd_msi_init,
>     ...
>   };
>
>   static struct msi_domain_info vmd_msi_domain_info = {
>     .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>              MSI_FLAG_PCI_MSIX,
>     .ops = &vmd_msi_domain_ops,
>     ...
>   };
>
> Both vmd_msi_domain_ops and vmd_msi_domain_info are statically
> initialized, but not completely.  Then we pass a pointer to
> pci_msi_create_irq_domain(), which fills in defaults for some of the
> function pointers that weren't statically initialized:
>
>   vmd_enable_domain()
>     pci_msi_create_irq_domain(NULL, &vmd_msi_domain_info, ...)
>       if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
>         pci_msi_domain_update_dom_ops(info)
>           if (ops->set_desc == NULL)
>             ops->msi_check = pci_msi_domain_check_cap
>
> We know at build-time what all the function pointers will be, so in
> principle we should be able to make the struct const, which would be
> even better than __ro_after_init.
>
> For example, we could require that callers set every function pointer
> before calling pci_msi_create_irq_domain(), using the default ones
> (pci_msi_domain_set_desc, pci_msi_domain_check_cap,
> pci_msi_domain_handle_error) if it doesn't need to override them,
> e.g.,
>
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_check = pci_msi_domain_check_cap,
>   };
>
> Or we could leave NULL pointers in the structure and have the code
> that calls through the function pointers check for NULL and call the
> default itself, e.g.,
>
>   if (ops->msi_check)
>     ops->msi_check(...)
>   else
>     pci_msi_domain_check_cap(...)
>
> It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
> the commits below.  I would CC: him for his thoughts, but I don't
> have a current email address.
>
>   aeeb59657c35 ("genirq: Provide default callbacks for msi_domain_ops")
>   3878eaefb89a ("PCI/MSI: Enhance core to support hierarchy irqdomain")

If we can do const, that would be preferred. That's generally easier
to reason about. I ended up doing this to the cdrom ops structure just
the other day:

http://www.openwall.com/lists/kernel-hardening/2017/02/14/2

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-15 20:46       ` Kees Cook
  0 siblings, 0 replies; 46+ messages in thread
From: Kees Cook @ 2017-02-15 20:46 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Thomas Gleixner, Marc Zyngier

On Wed, Feb 15, 2017 at 12:33 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Kees, Thomas, Marc]
>
> Hi Jess,
>
> Thanks for the patch!
>
> On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
>> Marked msi_domain_ops structs as __ro_after_init when called only during init.
>> This protects the data structure from accidental corruption.
>>
>> Suggested-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Jess Frazelle <me@jessfraz.com>
>> ---
>>  drivers/pci/host/pci-hyperv.c | 2 +-
>>  drivers/pci/host/vmd.c        | 2 +-
>>  drivers/pci/msi.c             | 2 +-
>
> I understand the value of __ro_after_init, but I'm not certain about
> sprinkling it around in seemingly random places because it's hard to
> know where to put it and whether we put it in all the right places.
>
> How did you choose these three files to change?  There are other
> instances of struct msi_domain_ops that use MSI_FLAG_USE_DEF_DOM_OPS.
> Should they be changed, too?  If not, is there a rule to figure out
> which ones should be made __ro_after_init?
>
> I wonder if adding __ro_after_init is really the best solution.  I
> looked at VMD to see how vmd_msi_domain_ops is updated.  Here are the
> definitions:
>
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_init  = vmd_msi_init,
>     ...
>   };
>
>   static struct msi_domain_info vmd_msi_domain_info = {
>     .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>              MSI_FLAG_PCI_MSIX,
>     .ops = &vmd_msi_domain_ops,
>     ...
>   };
>
> Both vmd_msi_domain_ops and vmd_msi_domain_info are statically
> initialized, but not completely.  Then we pass a pointer to
> pci_msi_create_irq_domain(), which fills in defaults for some of the
> function pointers that weren't statically initialized:
>
>   vmd_enable_domain()
>     pci_msi_create_irq_domain(NULL, &vmd_msi_domain_info, ...)
>       if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
>         pci_msi_domain_update_dom_ops(info)
>           if (ops->set_desc == NULL)
>             ops->msi_check = pci_msi_domain_check_cap
>
> We know at build-time what all the function pointers will be, so in
> principle we should be able to make the struct const, which would be
> even better than __ro_after_init.
>
> For example, we could require that callers set every function pointer
> before calling pci_msi_create_irq_domain(), using the default ones
> (pci_msi_domain_set_desc, pci_msi_domain_check_cap,
> pci_msi_domain_handle_error) if it doesn't need to override them,
> e.g.,
>
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_check = pci_msi_domain_check_cap,
>   };
>
> Or we could leave NULL pointers in the structure and have the code
> that calls through the function pointers check for NULL and call the
> default itself, e.g.,
>
>   if (ops->msi_check)
>     ops->msi_check(...)
>   else
>     pci_msi_domain_check_cap(...)
>
> It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
> the commits below.  I would CC: him for his thoughts, but I don't
> have a current email address.
>
>   aeeb59657c35 ("genirq: Provide default callbacks for msi_domain_ops")
>   3878eaefb89a ("PCI/MSI: Enhance core to support hierarchy irqdomain")

If we can do const, that would be preferred. That's generally easier
to reason about. I ended up doing this to the cdrom ops structure just
the other day:

http://www.openwall.com/lists/kernel-hardening/2017/02/14/2

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-15 20:46       ` Kees Cook
  0 siblings, 0 replies; 46+ messages in thread
From: Kees Cook @ 2017-02-15 20:46 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Thomas Gleixner, Marc Zyngier

On Wed, Feb 15, 2017 at 12:33 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Kees, Thomas, Marc]
>
> Hi Jess,
>
> Thanks for the patch!
>
> On Fri, Feb 10, 2017 at 05:37:56PM -0800, Jess Frazelle wrote:
>> Marked msi_domain_ops structs as __ro_after_init when called only during init.
>> This protects the data structure from accidental corruption.
>>
>> Suggested-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Jess Frazelle <me@jessfraz.com>
>> ---
>>  drivers/pci/host/pci-hyperv.c | 2 +-
>>  drivers/pci/host/vmd.c        | 2 +-
>>  drivers/pci/msi.c             | 2 +-
>
> I understand the value of __ro_after_init, but I'm not certain about
> sprinkling it around in seemingly random places because it's hard to
> know where to put it and whether we put it in all the right places.
>
> How did you choose these three files to change?  There are other
> instances of struct msi_domain_ops that use MSI_FLAG_USE_DEF_DOM_OPS.
> Should they be changed, too?  If not, is there a rule to figure out
> which ones should be made __ro_after_init?
>
> I wonder if adding __ro_after_init is really the best solution.  I
> looked at VMD to see how vmd_msi_domain_ops is updated.  Here are the
> definitions:
>
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_init  = vmd_msi_init,
>     ...
>   };
>
>   static struct msi_domain_info vmd_msi_domain_info = {
>     .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>              MSI_FLAG_PCI_MSIX,
>     .ops = &vmd_msi_domain_ops,
>     ...
>   };
>
> Both vmd_msi_domain_ops and vmd_msi_domain_info are statically
> initialized, but not completely.  Then we pass a pointer to
> pci_msi_create_irq_domain(), which fills in defaults for some of the
> function pointers that weren't statically initialized:
>
>   vmd_enable_domain()
>     pci_msi_create_irq_domain(NULL, &vmd_msi_domain_info, ...)
>       if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
>         pci_msi_domain_update_dom_ops(info)
>           if (ops->set_desc == NULL)
>             ops->msi_check = pci_msi_domain_check_cap
>
> We know at build-time what all the function pointers will be, so in
> principle we should be able to make the struct const, which would be
> even better than __ro_after_init.
>
> For example, we could require that callers set every function pointer
> before calling pci_msi_create_irq_domain(), using the default ones
> (pci_msi_domain_set_desc, pci_msi_domain_check_cap,
> pci_msi_domain_handle_error) if it doesn't need to override them,
> e.g.,
>
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_check = pci_msi_domain_check_cap,
>   };
>
> Or we could leave NULL pointers in the structure and have the code
> that calls through the function pointers check for NULL and call the
> default itself, e.g.,
>
>   if (ops->msi_check)
>     ops->msi_check(...)
>   else
>     pci_msi_domain_check_cap(...)
>
> It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
> the commits below.  I would CC: him for his thoughts, but I don't
> have a current email address.
>
>   aeeb59657c35 ("genirq: Provide default callbacks for msi_domain_ops")
>   3878eaefb89a ("PCI/MSI: Enhance core to support hierarchy irqdomain")

If we can do const, that would be preferred. That's generally easier
to reason about. I ended up doing this to the cdrom ops structure just
the other day:

http://www.openwall.com/lists/kernel-hardening/2017/02/14/2

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-15 20:33     ` [kernel-hardening] " Bjorn Helgaas
@ 2017-02-15 21:16       ` Thomas Gleixner
  -1 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-15 21:16 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

On Wed, 15 Feb 2017, Bjorn Helgaas wrote:
> We know at build-time what all the function pointers will be, so in
> principle we should be able to make the struct const, which would be
> even better than __ro_after_init.

Not everywhere unfortunately. In some instances it's a runtime decision, but
yes, they could be fixed. But there is a downside in doing this. See below.

> For example, we could require that callers set every function pointer
> before calling pci_msi_create_irq_domain(), using the default ones
> (pci_msi_domain_set_desc, pci_msi_domain_check_cap,
> pci_msi_domain_handle_error) if it doesn't need to override them,
> e.g.,
> 
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_check = pci_msi_domain_check_cap,
>   };
> 
> Or we could leave NULL pointers in the structure and have the code
> that calls through the function pointers check for NULL and call the
> default itself, e.g.,
> 
>   if (ops->msi_check)
>     ops->msi_check(...)
>   else
>     pci_msi_domain_check_cap(...)
> 
> It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
> the commits below.  I would CC: him for his thoughts, but I don't
> have a current email address.

Me neither :(

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.

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 have a look with Marc, what can be done in the long run.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-15 21:16       ` Thomas Gleixner
  0 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-15 21:16 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

On Wed, 15 Feb 2017, Bjorn Helgaas wrote:
> We know at build-time what all the function pointers will be, so in
> principle we should be able to make the struct const, which would be
> even better than __ro_after_init.

Not everywhere unfortunately. In some instances it's a runtime decision, but
yes, they could be fixed. But there is a downside in doing this. See below.

> For example, we could require that callers set every function pointer
> before calling pci_msi_create_irq_domain(), using the default ones
> (pci_msi_domain_set_desc, pci_msi_domain_check_cap,
> pci_msi_domain_handle_error) if it doesn't need to override them,
> e.g.,
> 
>   static struct msi_domain_ops vmd_msi_domain_ops = {
>     .get_hwirq = vmd_get_hwirq,
>     .msi_check = pci_msi_domain_check_cap,
>   };
> 
> Or we could leave NULL pointers in the structure and have the code
> that calls through the function pointers check for NULL and call the
> default itself, e.g.,
> 
>   if (ops->msi_check)
>     ops->msi_check(...)
>   else
>     pci_msi_domain_check_cap(...)
> 
> It looks like the "USE_DEF_OPS" framework was added by Jiang Liu with
> the commits below.  I would CC: him for his thoughts, but I don't
> have a current email address.

Me neither :(

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.

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 have a look with Marc, what can be done in the long run.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-15 21:16       ` [kernel-hardening] " Thomas Gleixner
@ 2017-02-16 14:35         ` Bjorn Helgaas
  -1 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-02-16 14:35 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

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.

> 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

Bjorn

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-16 14:35         ` Bjorn Helgaas
  0 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-02-16 14:35 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

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.

> 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

Bjorn

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-16 14:35         ` [kernel-hardening] " Bjorn Helgaas
@ 2017-02-16 14:38           ` Thomas Gleixner
  -1 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-16 14:38 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-02-16 14:38           ` Thomas Gleixner
  0 siblings, 0 replies; 46+ messages in thread
From: Thomas Gleixner @ 2017-02-16 14:38 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

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

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-02-16 14:38           ` [kernel-hardening] " Thomas Gleixner
@ 2017-03-07 19:07             ` Bjorn Helgaas
  -1 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-03-07 19:07 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

On Thu, Feb 16, 2017 at 03:38:05PM +0100, Thomas Gleixner wrote:
> 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.

This seems like a worthwhile change, so I don't want to just drop this
patch.  But if we're going to do something, I'd like to do it
everywhere that it makes sense, all at the same time.

It looks like the v2 series was split up by subsystem, which is fine
with me.  I'll happily apply the PCI parts (or ack them, since it
might make sense to apply all of them via the same non-PCI tree).

But I *would* like to include the following users of
MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS at the same
time (or explain why __ro_after_init won't work for them):

  pci-xgene-msi.c
  pcie-altera-msi.c
  pcie-iproc-msi.c
  pcie-xilinx-nwl.c

Bjorn

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-03-07 19:07             ` Bjorn Helgaas
  0 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-03-07 19:07 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Jess Frazelle, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

On Thu, Feb 16, 2017 at 03:38:05PM +0100, Thomas Gleixner wrote:
> 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.

This seems like a worthwhile change, so I don't want to just drop this
patch.  But if we're going to do something, I'd like to do it
everywhere that it makes sense, all at the same time.

It looks like the v2 series was split up by subsystem, which is fine
with me.  I'll happily apply the PCI parts (or ack them, since it
might make sense to apply all of them via the same non-PCI tree).

But I *would* like to include the following users of
MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS at the same
time (or explain why __ro_after_init won't work for them):

  pci-xgene-msi.c
  pcie-altera-msi.c
  pcie-iproc-msi.c
  pcie-xilinx-nwl.c

Bjorn

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-03-07 19:07             ` [kernel-hardening] " Bjorn Helgaas
@ 2017-03-14 18:50               ` Jessica Frazelle
  -1 siblings, 0 replies; 46+ messages in thread
From: Jessica Frazelle @ 2017-03-14 18:50 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Thomas Gleixner, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

I can update the patch series, sorry haven't had much time to devote
to this the past few weeks, but will update in the next day.

On Tue, Mar 7, 2017 at 11:07 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Feb 16, 2017 at 03:38:05PM +0100, Thomas Gleixner wrote:
>> 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.
>
> This seems like a worthwhile change, so I don't want to just drop this
> patch.  But if we're going to do something, I'd like to do it
> everywhere that it makes sense, all at the same time.
>
> It looks like the v2 series was split up by subsystem, which is fine
> with me.  I'll happily apply the PCI parts (or ack them, since it
> might make sense to apply all of them via the same non-PCI tree).
>
> But I *would* like to include the following users of
> MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS at the same
> time (or explain why __ro_after_init won't work for them):
>
>   pci-xgene-msi.c
>   pcie-altera-msi.c
>   pcie-iproc-msi.c
>   pcie-xilinx-nwl.c
>
> Bjorn



-- 


Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC  511E 18F3 685C 0022 BFF3
pgp.mit.edu

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-03-14 18:50               ` Jessica Frazelle
  0 siblings, 0 replies; 46+ messages in thread
From: Jessica Frazelle @ 2017-03-14 18:50 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Thomas Gleixner, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

I can update the patch series, sorry haven't had much time to devote
to this the past few weeks, but will update in the next day.

On Tue, Mar 7, 2017 at 11:07 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Thu, Feb 16, 2017 at 03:38:05PM +0100, Thomas Gleixner wrote:
>> 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.
>
> This seems like a worthwhile change, so I don't want to just drop this
> patch.  But if we're going to do something, I'd like to do it
> everywhere that it makes sense, all at the same time.
>
> It looks like the v2 series was split up by subsystem, which is fine
> with me.  I'll happily apply the PCI parts (or ack them, since it
> might make sense to apply all of them via the same non-PCI tree).
>
> But I *would* like to include the following users of
> MSI_FLAG_USE_DEF_DOM_OPS and MSI_FLAG_USE_DEF_CHIP_OPS at the same
> time (or explain why __ro_after_init won't work for them):
>
>   pci-xgene-msi.c
>   pcie-altera-msi.c
>   pcie-iproc-msi.c
>   pcie-xilinx-nwl.c
>
> Bjorn



-- 


Jessie Frazelle
4096R / D4C4 DD60 0D66 F65A 8EFC  511E 18F3 685C 0022 BFF3
pgp.mit.edu

^ permalink raw reply	[flat|nested] 46+ messages in thread

* Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
  2017-03-14 18:50               ` [kernel-hardening] " Jessica Frazelle
@ 2017-03-14 19:24                 ` Bjorn Helgaas
  -1 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-03-14 19:24 UTC (permalink / raw)
  To: Jessica Frazelle
  Cc: Thomas Gleixner, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

On Tue, Mar 14, 2017 at 11:50:50AM -0700, Jessica Frazelle wrote:
> I can update the patch series, sorry haven't had much time to devote
> to this the past few weeks, but will update in the next day.

Thanks, Jessica!  No problem, I know the feeling :)

Bjorn

^ permalink raw reply	[flat|nested] 46+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/5] pci: set msi_domain_ops as __ro_after_init
@ 2017-03-14 19:24                 ` Bjorn Helgaas
  0 siblings, 0 replies; 46+ messages in thread
From: Bjorn Helgaas @ 2017-03-14 19:24 UTC (permalink / raw)
  To: Jessica Frazelle
  Cc: Thomas Gleixner, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Bjorn Helgaas, Keith Busch,
	open list:Hyper-V CORE AND DRIVERS, open list:PCI SUBSYSTEM,
	open list, kernel-hardening, Kees Cook, Marc Zyngier

On Tue, Mar 14, 2017 at 11:50:50AM -0700, Jessica Frazelle wrote:
> I can update the patch series, sorry haven't had much time to devote
> to this the past few weeks, but will update in the next day.

Thanks, Jessica!  No problem, I know the feeling :)

Bjorn

^ permalink raw reply	[flat|nested] 46+ messages in thread

end of thread, other threads:[~2017-03-14 19:24 UTC | newest]

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-11  1:37 [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops as __ro_after_init Jess Frazelle
2017-02-11  1:37 ` [kernel-hardening] " Jess Frazelle
2017-02-11  1:37 ` [PATCH v2 2/5] time: mark syscore_ops " Jess Frazelle
2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
2017-02-11  2:12   ` John Stultz
2017-02-11  2:12     ` [kernel-hardening] " John Stultz
2017-02-11  9:23     ` Thomas Gleixner
2017-02-11  9:23       ` [kernel-hardening] " Thomas Gleixner
2017-02-11  1:37 ` [PATCH v2 3/5] pci: set msi_domain_ops " Jess Frazelle
2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
2017-02-12  4:08   ` KY Srinivasan
2017-02-12  4:08     ` [kernel-hardening] " KY Srinivasan
2017-02-12  4:08     ` KY Srinivasan
2017-02-13 18:14   ` Keith Busch
2017-02-13 18:14     ` [kernel-hardening] " Keith Busch
2017-02-15 20:33   ` Bjorn Helgaas
2017-02-15 20:33     ` [kernel-hardening] " Bjorn Helgaas
2017-02-15 20:46     ` Kees Cook
2017-02-15 20:46       ` [kernel-hardening] " Kees Cook
2017-02-15 20:46       ` Kees Cook
2017-02-15 21:16     ` Thomas Gleixner
2017-02-15 21:16       ` [kernel-hardening] " Thomas Gleixner
2017-02-16 14:35       ` Bjorn Helgaas
2017-02-16 14:35         ` [kernel-hardening] " Bjorn Helgaas
2017-02-16 14:38         ` Thomas Gleixner
2017-02-16 14:38           ` [kernel-hardening] " Thomas Gleixner
2017-03-07 19:07           ` Bjorn Helgaas
2017-03-07 19:07             ` [kernel-hardening] " Bjorn Helgaas
2017-03-14 18:50             ` Jessica Frazelle
2017-03-14 18:50               ` [kernel-hardening] " Jessica Frazelle
2017-03-14 19:24               ` Bjorn Helgaas
2017-03-14 19:24                 ` [kernel-hardening] " Bjorn Helgaas
2017-02-11  1:37 ` [PATCH v2 4/5] staging: " Jess Frazelle
2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
2017-02-11  1:37 ` [PATCH v2 5/5] x86: " Jess Frazelle
2017-02-11  1:37   ` [kernel-hardening] " Jess Frazelle
2017-02-11  9:14 ` [PATCH v2 1/5] irq: set {msi_domain,syscore}_ops " Thomas Gleixner
2017-02-11  9:14   ` [kernel-hardening] " Thomas Gleixner
2017-02-11  9:23   ` Thomas Gleixner
2017-02-11  9:23     ` [kernel-hardening] " Thomas Gleixner
2017-02-11 10:48   ` Jess Frazelle
2017-02-11 10:48     ` [kernel-hardening] " Jess Frazelle
2017-02-11 12:00     ` Thomas Gleixner
2017-02-11 12:00       ` [kernel-hardening] " Thomas Gleixner
2017-02-11 12:17       ` Jessica Frazelle
2017-02-11 12:17         ` [kernel-hardening] " Jessica Frazelle

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.