Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Sami Tolvanen <samitolvanen@google.com>
Cc: Neal Liu <neal.liu@mediatek.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>,
	Daniel Lezcano <daniel.lezcano@linaro.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>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	Linux PM <linux-pm@vger.kernel.org>,
	linux-tegra <linux-tegra@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	"moderated list:ARM/Mediatek SoC..." 
	<linux-mediatek@lists.infradead.org>,
	lkml <linux-kernel@vger.kernel.org>,
	wsd_upstream <wsd_upstream@mediatek.com>
Subject: Re: [PATCH v2] cpuidle: change enter_s2idle() prototype
Date: Fri, 24 Jul 2020 11:57:37 +0200
Message-ID: <CAJZ5v0g_14D-tyWFEZ9eOJC=GmzR-31iAAPff=Ch8KjFyK2wfw@mail.gmail.com> (raw)
In-Reply-To: <20200723190724.GA1339461@google.com>

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!

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

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='CAJZ5v0g_14D-tyWFEZ9eOJC=GmzR-31iAAPff=Ch8KjFyK2wfw@mail.gmail.com' \
    --to=rafael@kernel.org \
    --cc=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-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/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-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

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


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