* [PATCH v2] cpuidle: Fix CFI failure @ 2020-07-06 3:13 Neal Liu 2020-07-06 3:13 ` [PATCH v2] cpuidle: change enter_s2idle() prototype Neal Liu 0 siblings, 1 reply; 12+ messages in thread From: Neal Liu @ 2020-07-06 3:13 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 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 | 6 +++--- 4 files changed, 16 insertions(+), 10 deletions(-) -- 2.18.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-06 3:13 [PATCH v2] cpuidle: Fix CFI failure Neal Liu @ 2020-07-06 3:13 ` Neal Liu 2020-07-07 16:43 ` Sami Tolvanen 2020-07-09 12:18 ` Rafael J. Wysocki 0 siblings, 2 replies; 12+ messages in thread From: Neal Liu @ 2020-07-06 3:13 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> --- drivers/acpi/processor_idle.c | 6 ++++-- drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- drivers/idle/intel_idle.c | 6 ++++-- include/linux/cpuidle.h | 6 +++--- 4 files changed, 16 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..bee10c0 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -66,9 +66,9 @@ struct cpuidle_state { * suspended, so it must not re-enable interrupts at any point (even * temporarily) or attempt to change states of clock event devices. */ - 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] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-06 3:13 ` [PATCH v2] cpuidle: change enter_s2idle() prototype Neal Liu @ 2020-07-07 16:43 ` Sami Tolvanen 2020-07-09 12:18 ` Rafael J. Wysocki 1 sibling, 0 replies; 12+ messages in thread From: Sami Tolvanen @ 2020-07-07 16:43 UTC (permalink / raw) To: Neal Liu Cc: Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, linux-acpi, linux-pm, linux-tegra, linux-arm-kernel, linux-mediatek, lkml, wsd_upstream On Mon, Jul 06, 2020 at 11:13:16AM +0800, Neal Liu 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> > --- > drivers/acpi/processor_idle.c | 6 ++++-- > drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- > drivers/idle/intel_idle.c | 6 ++++-- > include/linux/cpuidle.h | 6 +++--- > 4 files changed, 16 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..bee10c0 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -66,9 +66,9 @@ struct cpuidle_state { > * suspended, so it must not re-enable interrupts at any point (even > * temporarily) or attempt to change states of clock event devices. > */ > - 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 This looks good to me, thank you for sending the patch! Please feel free to add: Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Sami ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-06 3:13 ` [PATCH v2] cpuidle: change enter_s2idle() prototype Neal Liu 2020-07-07 16:43 ` Sami Tolvanen @ 2020-07-09 12:18 ` Rafael J. Wysocki 2020-07-10 3:08 ` Neal Liu 1 sibling, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2020-07-09 12:18 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 6, 2020 at 5:13 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. Can you please explain this in a bit more detail? As it stands, I don't understand the problem statement enough to apply the patch. > Align function prototype of enter() since it needs return value for > some use cases. The return value of enter_s2idle() is no > need currently. So last time I requested you to document why ->enter_s2idle needs to return an int in the code, which has not been done. Please do that. > Signed-off-by: Neal Liu <neal.liu@mediatek.com> > --- > drivers/acpi/processor_idle.c | 6 ++++-- > drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- > drivers/idle/intel_idle.c | 6 ++++-- > include/linux/cpuidle.h | 6 +++--- > 4 files changed, 16 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..bee10c0 100644 > --- a/include/linux/cpuidle.h > +++ b/include/linux/cpuidle.h > @@ -66,9 +66,9 @@ struct cpuidle_state { > * suspended, so it must not re-enable interrupts at any point (even > * temporarily) or attempt to change states of clock event devices. > */ > - 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 [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-09 12:18 ` Rafael J. Wysocki @ 2020-07-10 3:08 ` Neal Liu 2020-07-20 8:21 ` Neal Liu 0 siblings, 1 reply; 12+ messages in thread From: Neal Liu @ 2020-07-10 3:08 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Neal Liu, 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 Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > On Mon, Jul 6, 2020 at 5:13 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. > > Can you please explain this in a bit more detail? > > As it stands, I don't understand the problem statement enough to apply > the patch. > Okay, Let's me try to explain more details. 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. There are multiple control flow instructions that could be manipulated by the attacker and subvert control flow. The target instructions that use data to determine the actual destination. - indirect jump - indirect call - return In this case, function prototype between caller and callee are mismatch. Caller: (type A)funcA Callee: (type A)funcB Callee: (type C)funcC funcA calls funcB -> no problem funcA calls funcC -> CFI check failed That's why we try to align function prototype. Please feel free to feedback if you have any questions. > > Align function prototype of enter() since it needs return value for > > some use cases. The return value of enter_s2idle() is no > > need currently. > > So last time I requested you to document why ->enter_s2idle needs to > return an int in the code, which has not been done. Please do that. > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com> > > --- > > drivers/acpi/processor_idle.c | 6 ++++-- > > drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- > > drivers/idle/intel_idle.c | 6 ++++-- > > include/linux/cpuidle.h | 6 +++--- > > 4 files changed, 16 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..bee10c0 100644 > > --- a/include/linux/cpuidle.h > > +++ b/include/linux/cpuidle.h > > @@ -66,9 +66,9 @@ struct cpuidle_state { > > * suspended, so it must not re-enable interrupts at any point (even > > * temporarily) or attempt to change states of clock event devices. > > */ > > - 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 [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-10 3:08 ` Neal Liu @ 2020-07-20 8:21 ` Neal Liu 2020-07-23 19:07 ` Sami Tolvanen 0 siblings, 1 reply; 12+ messages in thread From: Neal Liu @ 2020-07-20 8:21 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Neal Liu, 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 Gentle ping on this patch. On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote: > On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > > On Mon, Jul 6, 2020 at 5:13 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. > > > > Can you please explain this in a bit more detail? > > > > As it stands, I don't understand the problem statement enough to apply > > the patch. > > > > Okay, Let's me try to explain more details. > 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. > > There are multiple control flow instructions that could be manipulated > by the attacker and subvert control flow. The target instructions that > use data to determine the actual destination. > - indirect jump > - indirect call > - return > > In this case, function prototype between caller and callee are mismatch. > Caller: (type A)funcA > Callee: (type A)funcB > Callee: (type C)funcC > > funcA calls funcB -> no problem > funcA calls funcC -> CFI check failed > > That's why we try to align function prototype. > Please feel free to feedback if you have any questions. > > > > Align function prototype of enter() since it needs return value for > > > some use cases. The return value of enter_s2idle() is no > > > need currently. > > > > So last time I requested you to document why ->enter_s2idle needs to > > return an int in the code, which has not been done. Please do that. > > > > > Signed-off-by: Neal Liu <neal.liu@mediatek.com> > > > --- > > > drivers/acpi/processor_idle.c | 6 ++++-- > > > drivers/cpuidle/cpuidle-tegra.c | 8 +++++--- > > > drivers/idle/intel_idle.c | 6 ++++-- > > > include/linux/cpuidle.h | 6 +++--- > > > 4 files changed, 16 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..bee10c0 100644 > > > --- a/include/linux/cpuidle.h > > > +++ b/include/linux/cpuidle.h > > > @@ -66,9 +66,9 @@ struct cpuidle_state { > > > * suspended, so it must not re-enable interrupts at any point (even > > > * temporarily) or attempt to change states of clock event devices. > > > */ > > > - 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 [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-20 8:21 ` Neal Liu @ 2020-07-23 19:07 ` Sami Tolvanen 2020-07-24 9:57 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Sami Tolvanen @ 2020-07-23 19:07 UTC (permalink / raw) To: Neal Liu Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, ACPI Devel Maling List, Linux PM, linux-tegra, Linux ARM, moderated list:ARM/Mediatek SoC..., lkml, wsd_upstream On Mon, Jul 20, 2020 at 04:21:34PM +0800, Neal Liu wrote: > Gentle ping on this patch. > > > On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote: > > On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > > > On Mon, Jul 6, 2020 at 5:13 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. > > > > > > Can you please explain this in a bit more detail? > > > > > > As it stands, I don't understand the problem statement enough to apply > > > the patch. > > > > > > > Okay, Let's me try to explain more details. > > 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. > > > > There are multiple control flow instructions that could be manipulated > > by the attacker and subvert control flow. The target instructions that > > use data to determine the actual destination. > > - indirect jump > > - indirect call > > - return > > > > In this case, function prototype between caller and callee are mismatch. > > Caller: (type A)funcA > > Callee: (type A)funcB > > Callee: (type C)funcC > > > > funcA calls funcB -> no problem > > funcA calls funcC -> CFI check failed > > > > That's why we try to align function prototype. > > Please feel free to feedback if you have any questions. I think you should include a better explanation in the commit message. Perhaps something like this? init_state_node assigns the same callback function to both enter and enter_s2idle despite mismatching function types, which trips indirect call checking with Control-Flow Integrity (CFI). > > > > Align function prototype of enter() since it needs return value for > > > > some use cases. The return value of enter_s2idle() is no > > > > need currently. > > > > > > So last time I requested you to document why ->enter_s2idle needs to > > > return an int in the code, which has not been done. Please do that. Rafael, are you happy with the commit message documenting the reason, or would you prefer to also add a comment before enter_s2idle? Sami ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-23 19:07 ` Sami Tolvanen @ 2020-07-24 9:57 ` Rafael J. Wysocki 2020-07-24 10:24 ` Neal Liu 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2020-07-24 9:57 UTC (permalink / raw) To: Sami Tolvanen Cc: Neal Liu, Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, ACPI Devel Maling List, Linux PM, linux-tegra, Linux ARM, moderated list:ARM/Mediatek SoC..., lkml, wsd_upstream On Thu, Jul 23, 2020 at 9:07 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > On Mon, Jul 20, 2020 at 04:21:34PM +0800, Neal Liu wrote: > > Gentle ping on this patch. > > > > > > On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote: > > > On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > > > > On Mon, Jul 6, 2020 at 5:13 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. > > > > > > > > Can you please explain this in a bit more detail? > > > > > > > > As it stands, I don't understand the problem statement enough to apply > > > > the patch. > > > > > > > > > > Okay, Let's me try to explain more details. > > > 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. > > > > > > There are multiple control flow instructions that could be manipulated > > > by the attacker and subvert control flow. The target instructions that > > > use data to determine the actual destination. > > > - indirect jump > > > - indirect call > > > - return > > > > > > In this case, function prototype between caller and callee are mismatch. > > > Caller: (type A)funcA > > > Callee: (type A)funcB > > > Callee: (type C)funcC > > > > > > funcA calls funcB -> no problem > > > funcA calls funcC -> CFI check failed > > > > > > That's why we try to align function prototype. > > > Please feel free to feedback if you have any questions. > > I think you should include a better explanation in the commit message. > Perhaps something like this? > > init_state_node assigns the same callback function to both enter and > enter_s2idle despite mismatching function types, which trips indirect > call checking with Control-Flow Integrity (CFI). > > > > > > Align function prototype of enter() since it needs return value for > > > > > some use cases. The return value of enter_s2idle() is no > > > > > need currently. > > > > > > > > So last time I requested you to document why ->enter_s2idle needs to > > > > return an int in the code, which has not been done. Please do that. > > Rafael, are you happy with the commit message documenting the reason, > or would you prefer to also add a comment before enter_s2idle? As I said before, it would be good to have a comment in the code as well or people will be wondering why it is necessary to return anything from that callback, because its return value is never used. Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-24 9:57 ` Rafael J. Wysocki @ 2020-07-24 10:24 ` Neal Liu 2020-07-24 11:20 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Neal Liu @ 2020-07-24 10:24 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Neal Liu, Sami Tolvanen, Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, ACPI Devel Maling List, Linux PM, linux-tegra, Linux ARM, moderated list:ARM/Mediatek SoC..., lkml, wsd_upstream On Fri, 2020-07-24 at 11:57 +0200, Rafael J. Wysocki wrote: > On Thu, Jul 23, 2020 at 9:07 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > On Mon, Jul 20, 2020 at 04:21:34PM +0800, Neal Liu wrote: > > > Gentle ping on this patch. > > > > > > > > > On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote: > > > > On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > > > > > On Mon, Jul 6, 2020 at 5:13 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. > > > > > > > > > > Can you please explain this in a bit more detail? > > > > > > > > > > As it stands, I don't understand the problem statement enough to apply > > > > > the patch. > > > > > > > > > > > > > Okay, Let's me try to explain more details. > > > > 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. > > > > > > > > There are multiple control flow instructions that could be manipulated > > > > by the attacker and subvert control flow. The target instructions that > > > > use data to determine the actual destination. > > > > - indirect jump > > > > - indirect call > > > > - return > > > > > > > > In this case, function prototype between caller and callee are mismatch. > > > > Caller: (type A)funcA > > > > Callee: (type A)funcB > > > > Callee: (type C)funcC > > > > > > > > funcA calls funcB -> no problem > > > > funcA calls funcC -> CFI check failed > > > > > > > > That's why we try to align function prototype. > > > > Please feel free to feedback if you have any questions. > > > > I think you should include a better explanation in the commit message. > > Perhaps something like this? > > > > init_state_node assigns the same callback function to both enter and > > enter_s2idle despite mismatching function types, which trips indirect > > call checking with Control-Flow Integrity (CFI). > > > > > > > > Align function prototype of enter() since it needs return value for > > > > > > some use cases. The return value of enter_s2idle() is no > > > > > > need currently. > > > > > > > > > > So last time I requested you to document why ->enter_s2idle needs to > > > > > return an int in the code, which has not been done. Please do that. > > > > Rafael, are you happy with the commit message documenting the reason, > > or would you prefer to also add a comment before enter_s2idle? > > As I said before, it would be good to have a comment in the code as > well or people will be wondering why it is necessary to return > anything from that callback, because its return value is never used. > > Thanks! Is it okay to add these comments before enter_s2idle? /* * Align function type since init_state_node assigns the same callback * function to both enter and enter_s2idle despite mismatching function * types, which trips indirect call checking with Control-Flow Integrity * (CFI). */ int (*enter_s2idle)(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-24 10:24 ` Neal Liu @ 2020-07-24 11:20 ` Rafael J. Wysocki 2020-07-24 11:49 ` Neal Liu 0 siblings, 1 reply; 12+ messages in thread From: Rafael J. Wysocki @ 2020-07-24 11:20 UTC (permalink / raw) To: Neal Liu Cc: Rafael J. Wysocki, Sami Tolvanen, Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, ACPI Devel Maling List, Linux PM, linux-tegra, Linux ARM, moderated list:ARM/Mediatek SoC..., lkml, wsd_upstream On Fri, Jul 24, 2020 at 12:24 PM Neal Liu <neal.liu@mediatek.com> wrote: > > On Fri, 2020-07-24 at 11:57 +0200, Rafael J. Wysocki wrote: > > On Thu, Jul 23, 2020 at 9:07 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > On Mon, Jul 20, 2020 at 04:21:34PM +0800, Neal Liu wrote: > > > > Gentle ping on this patch. > > > > > > > > > > > > On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote: > > > > > On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > > > > > > On Mon, Jul 6, 2020 at 5:13 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. > > > > > > > > > > > > Can you please explain this in a bit more detail? > > > > > > > > > > > > As it stands, I don't understand the problem statement enough to apply > > > > > > the patch. > > > > > > > > > > > > > > > > Okay, Let's me try to explain more details. > > > > > 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. > > > > > > > > > > There are multiple control flow instructions that could be manipulated > > > > > by the attacker and subvert control flow. The target instructions that > > > > > use data to determine the actual destination. > > > > > - indirect jump > > > > > - indirect call > > > > > - return > > > > > > > > > > In this case, function prototype between caller and callee are mismatch. > > > > > Caller: (type A)funcA > > > > > Callee: (type A)funcB > > > > > Callee: (type C)funcC > > > > > > > > > > funcA calls funcB -> no problem > > > > > funcA calls funcC -> CFI check failed > > > > > > > > > > That's why we try to align function prototype. > > > > > Please feel free to feedback if you have any questions. > > > > > > I think you should include a better explanation in the commit message. > > > Perhaps something like this? > > > > > > init_state_node assigns the same callback function to both enter and > > > enter_s2idle despite mismatching function types, which trips indirect > > > call checking with Control-Flow Integrity (CFI). > > > > > > > > > > Align function prototype of enter() since it needs return value for > > > > > > > some use cases. The return value of enter_s2idle() is no > > > > > > > need currently. > > > > > > > > > > > > So last time I requested you to document why ->enter_s2idle needs to > > > > > > return an int in the code, which has not been done. Please do that. > > > > > > Rafael, are you happy with the commit message documenting the reason, > > > or would you prefer to also add a comment before enter_s2idle? > > > > As I said before, it would be good to have a comment in the code as > > well or people will be wondering why it is necessary to return > > anything from that callback, because its return value is never used. > > > > Thanks! > > Is it okay to add these comments before enter_s2idle? > > /* > * Align function type since init_state_node assigns the same callback init_state_node() > * function to both enter and enter_s2idle despite mismatching function ->enter_s2idle > * types, which trips indirect call checking with Control-Flow Integrity > * (CFI). > */ > int (*enter_s2idle)(struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index); But IMO it would be sufficient to add something like this to the existing comment regarding ->enter_s2idle: "This callback may point to the same function as ->enter if all of the above requirements are met by it." That would explain why the signature is the same sufficiently in my view. Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-24 11:20 ` Rafael J. Wysocki @ 2020-07-24 11:49 ` Neal Liu 2020-07-25 15:48 ` Rafael J. Wysocki 0 siblings, 1 reply; 12+ messages in thread From: Neal Liu @ 2020-07-24 11:49 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Neal Liu, Sami Tolvanen, Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, ACPI Devel Maling List, Linux PM, linux-tegra, Linux ARM, moderated list:ARM/Mediatek SoC..., lkml, wsd_upstream On Fri, 2020-07-24 at 13:20 +0200, Rafael J. Wysocki wrote: > On Fri, Jul 24, 2020 at 12:24 PM Neal Liu <neal.liu@mediatek.com> wrote: > > > > On Fri, 2020-07-24 at 11:57 +0200, Rafael J. Wysocki wrote: > > > On Thu, Jul 23, 2020 at 9:07 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > On Mon, Jul 20, 2020 at 04:21:34PM +0800, Neal Liu wrote: > > > > > Gentle ping on this patch. > > > > > > > > > > > > > > > On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote: > > > > > > On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > > > > > > > On Mon, Jul 6, 2020 at 5:13 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. > > > > > > > > > > > > > > Can you please explain this in a bit more detail? > > > > > > > > > > > > > > As it stands, I don't understand the problem statement enough to apply > > > > > > > the patch. > > > > > > > > > > > > > > > > > > > Okay, Let's me try to explain more details. > > > > > > 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. > > > > > > > > > > > > There are multiple control flow instructions that could be manipulated > > > > > > by the attacker and subvert control flow. The target instructions that > > > > > > use data to determine the actual destination. > > > > > > - indirect jump > > > > > > - indirect call > > > > > > - return > > > > > > > > > > > > In this case, function prototype between caller and callee are mismatch. > > > > > > Caller: (type A)funcA > > > > > > Callee: (type A)funcB > > > > > > Callee: (type C)funcC > > > > > > > > > > > > funcA calls funcB -> no problem > > > > > > funcA calls funcC -> CFI check failed > > > > > > > > > > > > That's why we try to align function prototype. > > > > > > Please feel free to feedback if you have any questions. > > > > > > > > I think you should include a better explanation in the commit message. > > > > Perhaps something like this? > > > > > > > > init_state_node assigns the same callback function to both enter and > > > > enter_s2idle despite mismatching function types, which trips indirect > > > > call checking with Control-Flow Integrity (CFI). > > > > > > > > > > > > Align function prototype of enter() since it needs return value for > > > > > > > > some use cases. The return value of enter_s2idle() is no > > > > > > > > need currently. > > > > > > > > > > > > > > So last time I requested you to document why ->enter_s2idle needs to > > > > > > > return an int in the code, which has not been done. Please do that. > > > > > > > > Rafael, are you happy with the commit message documenting the reason, > > > > or would you prefer to also add a comment before enter_s2idle? > > > > > > As I said before, it would be good to have a comment in the code as > > > well or people will be wondering why it is necessary to return > > > anything from that callback, because its return value is never used. > > > > > > Thanks! > > > > Is it okay to add these comments before enter_s2idle? > > > > /* > > * Align function type since init_state_node assigns the same callback > > init_state_node() > > > * function to both enter and enter_s2idle despite mismatching function > > ->enter_s2idle > > > * types, which trips indirect call checking with Control-Flow Integrity > > * (CFI). > > */ > > int (*enter_s2idle)(struct cpuidle_device *dev, > > struct cpuidle_driver *drv, > > int index); > > But IMO it would be sufficient to add something like this to the > existing comment regarding ->enter_s2idle: > > "This callback may point to the same function as ->enter if all of the > above requirements are met by it." > > That would explain why the signature is the same sufficiently in my view. > > Thanks! For clarification, do you mean add this comment on enter_s2idle function pointer declaration is enough? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] cpuidle: change enter_s2idle() prototype 2020-07-24 11:49 ` Neal Liu @ 2020-07-25 15:48 ` Rafael J. Wysocki 0 siblings, 0 replies; 12+ messages in thread From: Rafael J. Wysocki @ 2020-07-25 15:48 UTC (permalink / raw) To: Neal Liu Cc: Rafael J. Wysocki, Sami Tolvanen, Rafael J. Wysocki, Len Brown, Daniel Lezcano, Thierry Reding, Jonathan Hunter, Jacob Pan, Matthias Brugger, ACPI Devel Maling List, Linux PM, linux-tegra, Linux ARM, moderated list:ARM/Mediatek SoC..., lkml, wsd_upstream On Fri, Jul 24, 2020 at 1:50 PM Neal Liu <neal.liu@mediatek.com> wrote: > > On Fri, 2020-07-24 at 13:20 +0200, Rafael J. Wysocki wrote: > > On Fri, Jul 24, 2020 at 12:24 PM Neal Liu <neal.liu@mediatek.com> wrote: > > > > > > On Fri, 2020-07-24 at 11:57 +0200, Rafael J. Wysocki wrote: > > > > On Thu, Jul 23, 2020 at 9:07 PM Sami Tolvanen <samitolvanen@google.com> wrote: > > > > > > > > > > On Mon, Jul 20, 2020 at 04:21:34PM +0800, Neal Liu wrote: > > > > > > Gentle ping on this patch. > > > > > > > > > > > > > > > > > > On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote: > > > > > > > On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote: > > > > > > > > On Mon, Jul 6, 2020 at 5:13 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. > > > > > > > > > > > > > > > > Can you please explain this in a bit more detail? > > > > > > > > > > > > > > > > As it stands, I don't understand the problem statement enough to apply > > > > > > > > the patch. > > > > > > > > > > > > > > > > > > > > > > Okay, Let's me try to explain more details. > > > > > > > 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. > > > > > > > > > > > > > > There are multiple control flow instructions that could be manipulated > > > > > > > by the attacker and subvert control flow. The target instructions that > > > > > > > use data to determine the actual destination. > > > > > > > - indirect jump > > > > > > > - indirect call > > > > > > > - return > > > > > > > > > > > > > > In this case, function prototype between caller and callee are mismatch. > > > > > > > Caller: (type A)funcA > > > > > > > Callee: (type A)funcB > > > > > > > Callee: (type C)funcC > > > > > > > > > > > > > > funcA calls funcB -> no problem > > > > > > > funcA calls funcC -> CFI check failed > > > > > > > > > > > > > > That's why we try to align function prototype. > > > > > > > Please feel free to feedback if you have any questions. > > > > > > > > > > I think you should include a better explanation in the commit message. > > > > > Perhaps something like this? > > > > > > > > > > init_state_node assigns the same callback function to both enter and > > > > > enter_s2idle despite mismatching function types, which trips indirect > > > > > call checking with Control-Flow Integrity (CFI). > > > > > > > > > > > > > > Align function prototype of enter() since it needs return value for > > > > > > > > > some use cases. The return value of enter_s2idle() is no > > > > > > > > > need currently. > > > > > > > > > > > > > > > > So last time I requested you to document why ->enter_s2idle needs to > > > > > > > > return an int in the code, which has not been done. Please do that. > > > > > > > > > > Rafael, are you happy with the commit message documenting the reason, > > > > > or would you prefer to also add a comment before enter_s2idle? > > > > > > > > As I said before, it would be good to have a comment in the code as > > > > well or people will be wondering why it is necessary to return > > > > anything from that callback, because its return value is never used. > > > > > > > > Thanks! > > > > > > Is it okay to add these comments before enter_s2idle? > > > > > > /* > > > * Align function type since init_state_node assigns the same callback > > > > init_state_node() > > > > > * function to both enter and enter_s2idle despite mismatching function > > > > ->enter_s2idle > > > > > * types, which trips indirect call checking with Control-Flow Integrity > > > * (CFI). > > > */ > > > int (*enter_s2idle)(struct cpuidle_device *dev, > > > struct cpuidle_driver *drv, > > > int index); > > > > But IMO it would be sufficient to add something like this to the > > existing comment regarding ->enter_s2idle: > > > > "This callback may point to the same function as ->enter if all of the > > above requirements are met by it." > > > > That would explain why the signature is the same sufficiently in my view. > > > > Thanks! > > For clarification, do you mean add this comment on enter_s2idle function > pointer declaration is enough? Yes, I do. Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-07-25 15:48 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-06 3:13 [PATCH v2] cpuidle: Fix CFI failure Neal Liu 2020-07-06 3:13 ` [PATCH v2] cpuidle: change enter_s2idle() prototype Neal Liu 2020-07-07 16:43 ` Sami Tolvanen 2020-07-09 12:18 ` Rafael J. Wysocki 2020-07-10 3:08 ` Neal Liu 2020-07-20 8:21 ` Neal Liu 2020-07-23 19:07 ` Sami Tolvanen 2020-07-24 9:57 ` Rafael J. Wysocki 2020-07-24 10:24 ` Neal Liu 2020-07-24 11:20 ` Rafael J. Wysocki 2020-07-24 11:49 ` Neal Liu 2020-07-25 15:48 ` 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).