linux-kernel - RE: [PATCH] firmware: imx: Skip return value check for some special SCU firmware APIs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DB3PR0402MB391609C4C6A928E2E2F987C5F5820@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Mon, 30 Sep 2019 07:54:09 +0000
From:   Anson Huang <anson.huang@....com>
To:     Leonard Crestez <leonard.crestez@....com>,
        Marco Felsch <m.felsch@...gutronix.de>,
        Aisheng Dong <aisheng.dong@....com>
CC:     "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH] firmware: imx: Skip return value check for some special
 SCU firmware APIs

Hi, Leonard

> > On 2019-09-27 4:20 AM, Anson Huang wrote:
> > >> On 2019-09-26 1:06 PM, Marco Felsch wrote:
> > >>> On 19-09-26 08:03, Anson Huang wrote:
> > >>>>> On 19-09-25 18:07, Anson Huang wrote:
> > >>>>>> The SCU firmware does NOT always have return value stored in
> > >>>>>> message header's function element even the API has response
> > >>>>>> data, those special APIs are defined as void function in SCU
> > >>>>>> firmware, so they should be treated as return success always.
> > >>>>>>
> > >>>>>> +static const struct imx_sc_rpc_msg whitelist[] = {
> > >>>>>> +	{ .svc = IMX_SC_RPC_SVC_MISC, .func =
> > >>>>> IMX_SC_MISC_FUNC_UNIQUE_ID },
> > >>>>>> +	{ .svc = IMX_SC_RPC_SVC_MISC, .func =
> > >>>>>> +IMX_SC_MISC_FUNC_GET_BUTTON_STATUS }, };
> > >>>>>
> > >>>>> Is this going to be extended in the near future? I see some
> > >>>>> upcoming problems here if someone uses a different
> > >>>>> scu-fw<->kernel combination as nxp would suggest.
> > >>>>
> > >>>> Could be, but I checked the current APIs, ONLY these 2 will be
> > >>>> used in Linux kernel, so I ONLY add these 2 APIs for now.
> > >>>
> > >>> Okay.
> > >>>
> > >>>> However, after rethink, maybe we should add another imx_sc_rpc
> > >>>> API for those special APIs? To avoid checking it for all the APIs
> > >>>> called which
> > >> may impact some performance.
> > >>>> Still under discussion, if you have better idea, please advise, thanks!
> > >>
> > >> My suggestion is to refactor the code and add a new API for the
> > >> this "no error value" convention. Internally they can call a common
> > >> function with flags.
> > >
> > > If I understand your point correctly, that means the loop check of
> > > whether the API is with "no error value" for every API still NOT be
> > > skipped, it is just refactoring the code, right?
> >
> > There would be no "loop" anywhere: the responsibility would fall on
> > the call to call the right RPC function. In the current layering
> > scheme (drivers -> RPC ->
> > mailbox) the RPC layer treats all calls the same and it's up the the
> > caller to provide information about calling convention.
> >
> > An example implementation:
> > * Rename imx_sc_rpc_call to __imx_sc_rpc_call_flags
> > * Make a tiny imx_sc_rpc_call wrapper which just converts resp/noresp
> > to a flag
> > * Make get button status call __imx_sc_rpc_call_flags with the
> > _IMX_SC_RPC_NOERROR flag
> >
> > Hope this makes my suggestion clearer? Pushing this to the caller is a
> > bit ugly but I think it's worth preserving the fact that the imx rpc
> > core treats services in an uniform way.
> 
> It is clear now, so essentially it is same as 2 separate APIs, still need to change
> the button driver and uid driver to use the special flag, meanwhile, need to
> change the third parament of imx_sc_rpc_call() from bool to u32.

Correct one thing, no need to change the parameter of imx_sc_rpc_call(), just add
another API using the flag as parameter, and imx_sc_rpc_call() calls the new API.

Anson 

> 
> If no one opposes this approach, I will redo the patch together with the
> button driver and uid driver after holiday.
> 
> Anson