linux-kernel - Re: [PATCH v3] platform/chrome: Use proper protocol transfer function

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:   Tue, 19 Sep 2017 17:39:56 +0100
From:   Jon Hunter <jonathanh@...dia.com>
To:     Shawn N <shawnn@...omium.org>
CC:     Brian Norris <briannorris@...omium.org>,
        Olof Johansson <olof@...om.net>,
        Benson Leung <bleung@...omium.org>,
        Lee Jones <lee.jones@...aro.org>,
        <linux-kernel@...r.kernel.org>,
        Doug Anderson <dianders@...omium.org>,
        "Brian Norris" <computersforpeace@...il.com>,
        Javier Martinez Canillas <javier@....samsung.com>,
        Gwendal Grignou <gwendal@...omium.org>,
        "Enric Balletbo" <enric.balletbo@...labora.co.uk>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v3] platform/chrome: Use proper protocol transfer function



On 19/09/17 15:09, Shawn N wrote:
> On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@...dia.com> wrote:
>>
>> Hi Brian,
>>
>> On 08/09/17 21:50, Brian Norris wrote:
>>> From: Shawn Nematbakhsh <shawnn@...omium.org>
>>>
>>> pkt_xfer should be used for protocol v3, and cmd_xfer otherwise. We had
>>> one instance of these functions correct, but not the second, fall-back
>>> case. We use the fall-back only when the first command returns an
>>> IN_PROGRESS status, which is only used on some EC firmwares where we
>>> don't want to constantly poll the bus, but instead back off and
>>> sleep/retry for a little while.
>>>
>>> Fixes: 2c7589af3c4d ("mfd: cros_ec: add proto v3 skeleton")
>>> Signed-off-by: Shawn Nematbakhsh <shawnn@...omium.org>
>>> Signed-off-by: Brian Norris <briannorris@...omium.org>
>>> Reviewed-by: Javier Martinez Canillas <javier@....samsung.com>
>>> ---
>>> v3:
>>>  * Added Javier's reviewed tag
>>>  * It's been > 8 months since [1], so why not? And hey, Benson's officially in
>>>    MAINTAINERS now! Too bad no one told me.
>>>
>>>  [1] https://patchwork.kernel.org/patch/9450633/
>>>
>>>
>>> v2:
>>>  * Add Benson in 'To:'
>>>  * make subject prefix more obvious
>>>
>>>  drivers/platform/chrome/cros_ec_proto.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>>> index 8dfa7fcb1248..e7bbdf947bbc 100644
>>> --- a/drivers/platform/chrome/cros_ec_proto.c
>>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>>> @@ -60,12 +60,14 @@ static int send_command(struct cros_ec_device *ec_dev,
>>>                       struct cros_ec_command *msg)
>>>  {
>>>       int ret;
>>> +     int (*xfer_fxn)(struct cros_ec_device *ec, struct cros_ec_command *msg);
>>>
>>>       if (ec_dev->proto_version > 2)
>>> -             ret = ec_dev->pkt_xfer(ec_dev, msg);
>>> +             xfer_fxn = ec_dev->pkt_xfer;
>>>       else
>>> -             ret = ec_dev->cmd_xfer(ec_dev, msg);
>>> +             xfer_fxn = ec_dev->cmd_xfer;u
>>>
>>> +     ret = (*xfer_fxn)(ec_dev, msg);
>>>       if (msg->result == EC_RES_IN_PROGRESS) {
>>>               int i;
>>>               struct cros_ec_command *status_msg;
>>> @@ -88,7 +90,7 @@ static int send_command(struct cros_ec_device *ec_dev,
>>>               for (i = 0; i < EC_COMMAND_RETRIES; i++) {
>>>                       usleep_range(10000, 11000);
>>>
>>> -                     ret = ec_dev->cmd_xfer(ec_dev, status_msg);
>>> +                     ret = (*xfer_fxn)(ec_dev, status_msg);
>>>                       if (ret < 0)
>>>                               break;
>>>
>>
>> Tegra124 Nyan-Big is currently crashing during boot with -next [0] and
>> bisect is pointing to this commit. Reverting the above on top of -next
>> does allow the board to boot successfully. Looks like this board is
>> proto_version 3 but I have not looked into this any further. Let me know
>> if you have any thoughts.
> 
> 
> Thanks for the bug report, I'll look into this today.
> 
>> [    1.502497] kernel BUG at drivers/platform/chrome/cros_ec_proto.c:34!
>> 34 BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
> 
> So, ec_dev->proto_version > 3? That doesn't seem right.

You mean != 3, but yes. Looks like an initialisation problem, because if I 
add the following WARNING ...

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index e7bbdf947bbc..ad3b3a1e8d54 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -31,6 +31,7 @@ static int prepare_packet(struct cros_ec_device *ec_dev,
        int i;
        u8 csum = 0;
 
+       WARN(ec_dev->proto_version != EC_HOST_REQUEST_VERSION, "%d != %d", ec_dev->proto_version, EC_HOST_REQUEST_VERSION);
        BUG_ON(ec_dev->proto_version != EC_HOST_REQUEST_VERSION);
        BUG_ON(msg->outsize + sizeof(*request) > ec_dev->dout_size);
 
... then I see ...

[    1.502495] WARNING: CPU: 0 PID: 1 at drivers/platform/chrome/cros_ec_proto.c:35 cros_ec_prepare_tx+0x190/0x1a8
[    1.512566] 65535 != 3

Any chance this is being called before the version is initialised?

Cheers
Jon

-- 
nvpublic