* [PATCH v3] cpuidle: Fix CFI failure @ 2020-07-27 3:25 Neal Liu 2020-07-27 3:25 ` [PATCH v3] cpuidle: change enter_s2idle() prototype Neal Liu 0 siblings, 1 reply; 3+ messages in thread From: Neal Liu @ 2020-07-27 3:25 UTC (permalink / raw) To: Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, Sami Tolvanen Cc: Neal Liu, linux-acpi, linux-pm, linux-tegra, linux-arm-kernel, linux-mediatek, lkml, wsd_upstream changes since v2: - add more comments on enter_s2idle to explain why it is necessary to return int even if its return value is never used. changes since v1: - add more description in commit message. *** BLURB HERE *** Neal Liu (1): cpuidle: change enter_s2idle() prototype drivers/acpi/processor_idle.c | 6 ++++-- drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- drivers/idle/intel_idle.c | 6 ++++-- include/linux/cpuidle.h | 9 ++++++--- 4 files changed, 19 insertions(+), 10 deletions(-) -- 2.18.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v3] cpuidle: change enter_s2idle() prototype 2020-07-27 3:25 [PATCH v3] cpuidle: Fix CFI failure Neal Liu @ 2020-07-27 3:25 ` Neal Liu 2020-07-27 12:09 ` Rafael J. Wysocki 0 siblings, 1 reply; 3+ messages in thread From: Neal Liu @ 2020-07-27 3:25 UTC (permalink / raw) To: Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, Sami Tolvanen Cc: Neal Liu, linux-acpi, linux-pm, linux-tegra, linux-arm-kernel, linux-mediatek, lkml, wsd_upstream Control Flow Integrity(CFI) is a security mechanism that disallows changes to the original control flow graph of a compiled binary, making it significantly harder to perform such attacks. init_state_node() assign same function callback to different function pointer declarations. static int init_state_node(struct cpuidle_state *idle_state, const struct of_device_id *matches, struct device_node *state_node) { ... idle_state->enter = match_id->data; ... idle_state->enter_s2idle = match_id->data; } Function declarations: struct cpuidle_state { ... int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); void (*enter_s2idle) (struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); }; In this case, either enter() or enter_s2idle() would cause CFI check failed since they use same callee. Align function prototype of enter() since it needs return value for some use cases. The return value of enter_s2idle() is no need currently. Signed-off-by: Neal Liu <neal.liu@mediatek.com> Reviewed-by: Sami Tolvanen <samitolvanen@google.com> --- drivers/acpi/processor_idle.c | 6 ++++-- drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- drivers/idle/intel_idle.c | 6 ++++-- include/linux/cpuidle.h | 9 ++++++--- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c index 75534c5..6ffb6c9 100644 --- a/drivers/acpi/processor_idle.c +++ b/drivers/acpi/processor_idle.c @@ -655,8 +655,8 @@ static int acpi_idle_enter(struct cpuidle_device *dev, return index; } -static void acpi_idle_enter_s2idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) +static int acpi_idle_enter_s2idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) { struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu); @@ -674,6 +674,8 @@ static void acpi_idle_enter_s2idle(struct cpuidle_device *dev, } } acpi_idle_do_entry(cx); + + return 0; } static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr, diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c index 1500458..a12fb14 100644 --- a/drivers/cpuidle/cpuidle-tegra.c +++ b/drivers/cpuidle/cpuidle-tegra.c @@ -253,11 +253,13 @@ static int tegra_cpuidle_enter(struct cpuidle_device *dev, return err ? -1 : index; } -static void tegra114_enter_s2idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index) +static int tegra114_enter_s2idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) { tegra_cpuidle_enter(dev, drv, index); + + return 0; } /* diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c index f449584..b178da3 100644 --- a/drivers/idle/intel_idle.c +++ b/drivers/idle/intel_idle.c @@ -175,13 +175,15 @@ static __cpuidle int intel_idle(struct cpuidle_device *dev, * Invoked as a suspend-to-idle callback routine with frozen user space, frozen * scheduler tick and suspended scheduler clock on the target CPU. */ -static __cpuidle void intel_idle_s2idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) +static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) { unsigned long eax = flg2MWAIT(drv->states[index].flags); unsigned long ecx = 1; /* break on interrupt flag */ mwait_idle_with_hints(eax, ecx); + + return 0; } /* diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index ec2ef63..b65909a 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -65,10 +65,13 @@ struct cpuidle_state { * CPUs execute ->enter_s2idle with the local tick or entire timekeeping * suspended, so it must not re-enable interrupts at any point (even * temporarily) or attempt to change states of clock event devices. + * + * This callback may point to the same function as ->enter if all of + * the above requirements are met by it. */ - void (*enter_s2idle) (struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index); + int (*enter_s2idle)(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index); }; /* Idle State Flags */ -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] cpuidle: change enter_s2idle() prototype 2020-07-27 3:25 ` [PATCH v3] cpuidle: change enter_s2idle() prototype Neal Liu @ 2020-07-27 12:09 ` Rafael J. Wysocki 0 siblings, 0 replies; 3+ messages in thread From: Rafael J. Wysocki @ 2020-07-27 12:09 UTC (permalink / raw) To: Neal Liu Cc: Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, Sami Tolvanen, ACPI Devel Maling List, Linux PM, linux-tegra, Linux ARM, moderated list:ARM/Mediatek SoC..., lkml, wsd_upstream On Mon, Jul 27, 2020 at 5:25 AM Neal Liu <neal.liu@mediatek.com> wrote: > > Control Flow Integrity(CFI) is a security mechanism that disallows > changes to the original control flow graph of a compiled binary, > making it significantly harder to perform such attacks. > > init_state_node() assign same function callback to different > function pointer declarations. > > static int init_state_node(struct cpuidle_state *idle_state, > const struct of_device_id *matches, > struct device_node *state_node) { ... > idle_state->enter = match_id->data; ... > idle_state->enter_s2idle = match_id->data; } > > Function declarations: > > struct cpuidle_state { ... > int (*enter) (struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index); > > void (*enter_s2idle) (struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index); }; > > In this case, either enter() or enter_s2idle() would cause CFI check > failed since they use same callee. > > Align function prototype of enter() since it needs return value for > some use cases. The return value of enter_s2idle() is no > need currently. > > Signed-off-by: Neal Liu <neal.liu@mediatek.com> > Reviewed-by: Sami Tolvanen <samitolvanen@google.com> > --- > drivers/acpi/processor_idle.c | 6 ++++-- > drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- > drivers/idle/intel_idle.c | 6 ++++-- > include/linux/cpuidle.h | 9 ++++++--- > 4 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c > index 75534c5..6ffb6c9 100644 > --- a/drivers/acpi/processor_idle.c > +++ b/drivers/acpi/processor_idle.c > @@ -655,8 +655,8 @@ static int acpi_idle_enter(struct cpuidle_device *dev, > return index; > } > > -static void acpi_idle_enter_s2idle(struct cpuidle_device *dev, > - struct cpuidle_driver *drv, int index) > +static int acpi_idle_enter_s2idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > { > struct acpi_processor_cx *cx = per_cpu(acpi_cstate[index], dev->cpu); > > @@ -674,6 +674,8 @@ static void acpi_idle_enter_s2idle(struct cpuidle_device *dev, > } > } > acpi_idle_do_entry(cx); > + > + return 0; > } > > static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr, > diff --git a/drivers/cpuidle/cpuidle-tegra.c b/drivers/cpuidle/cpuidle-tegra.c > index 1500458..a12fb14 100644 > --- a/drivers/cpuidle/cpuidle-tegra.c > +++ b/drivers/cpuidle/cpuidle-tegra.c > @@ -253,11 +253,13 @@ static int tegra_cpuidle_enter(struct cpuidle_device *dev, > return err ? -1 : index; > } > > -static void tegra114_enter_s2idle(struct cpuidle_device *dev, > - struct cpuidle_driver *drv, > - int index) > +static int tegra114_enter_s2idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, > + int index) > { > tegra_cpuidle_enter(dev, drv, index); > + > + return 0; > } > > /* > diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c > index f449584..b178da3 100644 > --- a/drivers/idle/intel_idle.c > +++ b/drivers/idle/intel_idle.c > @@ -175,13 +175,15 @@ static __cpuidle int intel_idle(struct cpuidle_device *dev, > * Invoked as a suspend-to-idle callback routine with frozen user space, frozen > * scheduler tick and suspended scheduler clock on the target CPU. > */ > -static __cpuidle void intel_idle_s2idle(struct cpuidle_device *dev, > - struct cpuidle_driver *drv, int index) > +static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > { > unsigned long eax = flg2MWAIT(drv->states[index].flags); > unsigned long ecx = 1; /* break on interrupt flag */ > > mwait_idle_with_hints(eax, ecx); > + > + return 0; > } > > /* > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > index ec2ef63..b65909a 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -65,10 +65,13 @@ struct cpuidle_state { > * CPUs execute ->enter_s2idle with the local tick or entire timekeeping > * suspended, so it must not re-enable interrupts at any point (even > * temporarily) or attempt to change states of clock event devices. > + * > + * This callback may point to the same function as ->enter if all of > + * the above requirements are met by it. > */ > - void (*enter_s2idle) (struct cpuidle_device *dev, > - struct cpuidle_driver *drv, > - int index); > + int (*enter_s2idle)(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, > + int index); > }; > > /* Idle State Flags */ > -- Applied as 5.9 material, thanks! ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-27 12:10 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-27 3:25 [PATCH v3] cpuidle: Fix CFI failure Neal Liu 2020-07-27 3:25 ` [PATCH v3] cpuidle: change enter_s2idle() prototype Neal Liu 2020-07-27 12:09 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).