Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Neal Liu <neal.liu@mediatek.com>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	Jonathan Hunter <jonathanh@nvidia.com>,
	Jacob Pan <jacob.jun.pan@linux.intel.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, wsd_upstream@mediatek.com
Subject: Re: [PATCH] cpuidle: change enter_s2idle() prototype
Date: Wed, 1 Jul 2020 07:50:56 +0200
Message-ID: <a65bb456-d5f4-2208-f924-0af989b8dd5f@linaro.org> (raw)
In-Reply-To: <1593571181.7383.5.camel@mtkswgap22>

On 01/07/2020 04:39, Neal Liu wrote:
> On Mon, 2020-06-29 at 17:17 +0200, Rafael J. Wysocki wrote:
>> On Monday, June 29, 2020 11:05:40 AM CEST 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() assigns same function pointer to idle_state->enter
>>> and idle_state->enter_s2idle. This definitely causes CFI failure
>>> when calling either enter() or enter_s2idle().
>>>
>>> Align enter_s2idle() with enter() function prototype to fix CFI
>>> failure.
>>
>> That needs to be documented somewhere close to the definition of the
>> callbacks in question.
>>
>> Otherwise it is completely unclear why this is a good idea.
>>
> 
> The problem is, init_state_mode() 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.
> 
> We try to align function prototype of enter() since it needs return
> value for some use cases. The return value of enter_s2idle() is no need
> currently.

Thanks for the clarification, you may add this description along with
the changelog.


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


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1593421540-7397-1-git-send-email-neal.liu@mediatek.com>
     [not found] ` <1593421540-7397-2-git-send-email-neal.liu@mediatek.com>
2020-06-29 15:17   ` Rafael J. Wysocki
2020-07-01  2:39     ` Neal Liu
2020-07-01  5:50       ` Daniel Lezcano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a65bb456-d5f4-2208-f924-0af989b8dd5f@linaro.org \
    --to=daniel.lezcano@linaro.org \
    --cc=jacob.jun.pan@linux.intel.com \
    --cc=jonathanh@nvidia.com \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=matthias.bgg@gmail.com \
    --cc=neal.liu@mediatek.com \
    --cc=rjw@rjwysocki.net \
    --cc=samitolvanen@google.com \
    --cc=thierry.reding@gmail.com \
    --cc=wsd_upstream@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git