Re: [PATCH v2] cpuidle: change enter_s2idle() prototype

On Fri, 2020-07-24 at 13:20 +0200, Rafael J. Wysocki wrote:

On Fri, Jul 24, 2020 at 12:24 PM Neal Liu [off-list ref] wrote:

quoted

On Fri, 2020-07-24 at 11:57 +0200, Rafael J. Wysocki wrote:

quoted

On Thu, Jul 23, 2020 at 9:07 PM Sami Tolvanen [off-list ref] wrote:

quoted

On Mon, Jul 20, 2020 at 04:21:34PM +0800, Neal Liu wrote:

quoted

Gentle ping on this patch.


On Fri, 2020-07-10 at 11:08 +0800, Neal Liu wrote:

quoted

On Thu, 2020-07-09 at 14:18 +0200, Rafael J. Wysocki wrote:

quoted

On Mon, Jul 6, 2020 at 5:13 AM Neal Liu [off-list ref] wrote:

quoted

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

quoted

quoted

quoted

quoted

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

quoted

 * function to both enter and enter_s2idle despite mismatching function

->enter_s2idle

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel