[kernel-hardening] [PATCH 0/4] set {msi_domain,syscore}_ops as __ro_after_init

[kernel-hardening] [PATCH 0/4] set {msi_domain,syscore}_ops as __ro_after_init

[Jess Frazelle]

This is an initial pass and sanity check to make sure this is correct. The
changes have all been tested on x86.

Best,
Jess

[kernel-hardening] [PATCH 1/4] irq: set {msi_domain,syscore}_ops as __ro_after_init

[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.
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 +-
 kernel/irq/generic-chip.c                              | 2 +-
 kernel/irq/msi.c                                       | 2 +-
 kernel/irq/pm.c                                        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

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,
 };

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

Re: [kernel-hardening] [PATCH 1/4] irq: set {msi_domain,syscore}_ops as __ro_after_init

[Laura Abbott]

On 02/10/2017 02:08 AM, 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.
> 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 +-
>  kernel/irq/generic-chip.c                              | 2 +-
>  kernel/irq/msi.c                                       | 2 +-
>  kernel/irq/pm.c                                        | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> 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,
>  };
> 

The staging change is probably better suited to its own patch
since staging drivers tend to get handled differently.

Thanks,
Laura

> 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
> 

Re: [kernel-hardening] [PATCH 1/4] irq: set {msi_domain,syscore}_ops as __ro_after_init

[Jessica Frazelle]

Got it, thanks!

On Fri, Feb 10, 2017 at 12:26 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 02/10/2017 02:08 AM, 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.
> > 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 +-
> >  kernel/irq/generic-chip.c                              | 2 +-
> >  kernel/irq/msi.c                                       | 2 +-
> >  kernel/irq/pm.c                                        | 2 +-
> >  4 files changed, 4 insertions(+), 4 deletions(-)
> >
> > 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,
> >  };
> >
>
> The staging change is probably better suited to its own patch
> since staging drivers tend to get handled differently.
>
> Thanks,
> Laura
>
> > 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
> >
>

Re: [kernel-hardening] [PATCH 1/4] irq: set {msi_domain,syscore}_ops as __ro_after_init

[Kees Cook]

On Fri, Feb 10, 2017 at 2:08 AM, Jess Frazelle <me@jessfraz.com> 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.
> 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 +-
>  kernel/irq/generic-chip.c                              | 2 +-
>  kernel/irq/msi.c                                       | 2 +-
>  kernel/irq/pm.c                                        | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
>
> 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,
>  };
>
> 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,
>  };

Cool! How'd you end up choosing these? Did you just go looking for
one-sided initializations? (i.e. register_syscore_ops() without
unregister_syscore_ops() call?)

(It may help the commit message to explicitly state that
unregister_syscore_ops() is never called on these ops.)

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] [PATCH 1/4] irq: set {msi_domain,syscore}_ops as __ro_after_init

[Jessica Frazelle]

On Fri, Feb 10, 2017 at 1:55 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Feb 10, 2017 at 2:08 AM, Jess Frazelle <me@jessfraz.com> 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.
>> 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 +-
>>  kernel/irq/generic-chip.c                              | 2 +-
>>  kernel/irq/msi.c                                       | 2 +-
>>  kernel/irq/pm.c                                        | 2 +-
>>  4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> 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,
>>  };
>>
>> 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,
>>  };
>
> Cool! How'd you end up choosing these? Did you just go looking for
> one-sided initializations? (i.e. register_syscore_ops() without
> unregister_syscore_ops() call?)

I first made the mistake of making them all __ro_after_init and I
broke the kvm module, so I had to track down how that happened since I
didn't touch that code path... I ended up digging a bit into the call
stack that ultimately leads to these and this is my initial "safe
set". I am still digging into the others. But almost all the functions
that call `syscore_ops` structs were already marked as __init.

>
> (It may help the commit message to explicitly state that
> unregister_syscore_ops() is never called on these ops.)

Will fix!

>
> -Kees
>
> --
> Kees Cook
> Pixel Security



-- 


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

Re: [kernel-hardening] [PATCH 1/4] irq: set {msi_domain,syscore}_ops as __ro_after_init

[Kees Cook]

On Fri, Feb 10, 2017 at 2:02 PM, Jessica Frazelle <me@jessfraz.com> wrote:
> On Fri, Feb 10, 2017 at 1:55 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Fri, Feb 10, 2017 at 2:08 AM, Jess Frazelle <me@jessfraz.com> 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.
>>> 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 +-
>>>  kernel/irq/generic-chip.c                              | 2 +-
>>>  kernel/irq/msi.c                                       | 2 +-
>>>  kernel/irq/pm.c                                        | 2 +-
>>>  4 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> 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,
>>>  };
>>>
>>> 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,
>>>  };
>>
>> Cool! How'd you end up choosing these? Did you just go looking for
>> one-sided initializations? (i.e. register_syscore_ops() without
>> unregister_syscore_ops() call?)
>
> I first made the mistake of making them all __ro_after_init and I
> broke the kvm module, so I had to track down how that happened since I
> didn't touch that code path... I ended up digging a bit into the call
> stack that ultimately leads to these and this is my initial "safe
> set". I am still digging into the others. But almost all the functions
> that call `syscore_ops` structs were already marked as __init.

Right, the danger is if something outside of __init tries to change
the structure, but none of these seem to ever have an UNregister call
made against them. :)

>> (It may help the commit message to explicitly state that
>> unregister_syscore_ops() is never called on these ops.)
>
> Will fix!

Awesome. :)

-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] [PATCH 2/4] pci: set msi_domain_ops as __ro_after_init

[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

[kernel-hardening] [PATCH 3/4] x86: set msi_domain_ops as __ro_after_init

[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>
---
 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

[kernel-hardening] [PATCH 4/4] time: mark syscore_ops as __ro_after_init

[Jess Frazelle]

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.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Jess Frazelle <me@jessfraz.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

Re: [kernel-hardening] [PATCH 4/4] time: mark syscore_ops as __ro_after_init

[Rik van Riel]

On Fri, 2017-02-10 at 02:09 -0800, Jess Frazelle 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.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Jess Frazelle <me@jessfraz.com>

Acked-by: Rik van Riel <riel@redhat.com>

Re: [kernel-hardening] [PATCH 0/4] set {msi_domain,syscore}_ops as __ro_after_init

[Anisse Astier]

Hi Jess,

On Fri, Feb 10, 2017 at 02:08:58AM -0800, Jess Frazelle wrote:
> This is an initial pass and sanity check to make sure this is correct. The
> changes have all been tested on x86.

May I suggested for the next iteration that you Cc: the
maintainers/lists for each affected subsystem ?

Regards,

Anisse

Re: [kernel-hardening] [PATCH 0/4] set {msi_domain,syscore}_ops as __ro_after_init

[Jessica Frazelle]

Thanks, ya I had planned on it, just didn't want to do that if this was wrong :)

On Fri, Feb 10, 2017 at 2:23 AM, Anisse Astier <anisse@astier.eu> wrote:
> Hi Jess,
>
> On Fri, Feb 10, 2017 at 02:08:58AM -0800, Jess Frazelle wrote:
>> This is an initial pass and sanity check to make sure this is correct. The
>> changes have all been tested on x86.
>
> May I suggested for the next iteration that you Cc: the
> maintainers/lists for each affected subsystem ?
>
> Regards,
>
> Anisse
>



-- 


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

Re: [kernel-hardening] [PATCH 0/4] set {msi_domain,syscore}_ops as __ro_after_init

[Laura Abbott]

On 02/10/2017 02:08 AM, Jess Frazelle wrote:
> This is an initial pass and sanity check to make sure this is correct. The
> changes have all been tested on x86.
> 
> Best,
> Jess
> 

Looks good to me. I also boot tested this on arm64 as well. I'll give
an actual Tested-by when you send the follow on series.

Thanks,
Laura