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

[Sami Tolvanen <samitolvanen@google.com>]

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