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]

Date:   Thu, 26 Sep 2019 13:25:50 +0000
From:   Leonard Crestez <leonard.crestez@....com>
To:     Marco Felsch <m.felsch@...gutronix.de>,
        Anson Huang <anson.huang@....com>,
        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

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.

> Adding a special api shouldn't be the right fix. Imagine if someone (not
> a nxp-developer) wants to add a new driver. How could he be expected to
> know which api he should use. The better abbroach would be to fix the
> scu-fw instead of adding quirks..

Right now developers who want to make SCFW calls in upstream need to 
define the message struct in their driver based on protocol 
documentation. This includes:

* Binary layout of the message (a packed struct)
* If the message has a response (already a bool flag)
* If an error code is returned (this patch adds support for it)

Since callers are already exposed to the binary protocol exposing them 
to minor quirks of the calling convention also seems reasonable. Having 
the low-level IPC code peek at message IDs seems like a hack; this 
belong at a slightly higher level.

In older internal trees we use a very large amount of generated wrapper 
functions. This hides calling convention details from callers but is 
extremely ugly and verbose.

--
Regards,
Leonard