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 10:14:01 -0700
From:   Brian Norris <briannorris@...omium.org>
To:     Jon Hunter <jonathanh@...dia.com>
Cc:     Shawn N <shawnn@...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

Hi Jon,

On Tue, Sep 19, 2017 at 05:39:56PM +0100, Jon Hunter wrote:
> On 19/09/17 15:09, Shawn N wrote:
> > On Tue, Sep 19, 2017 at 6:44 AM, Jon Hunter <jonathanh@...dia.com> wrote:
> >> 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.

Yes, thanks!

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

If it's uninitialized, it should be 0 (the structure is kzalloc'd, and
the call stack you point to clearly shows it's at least been allocated
already). Also, if it's uninitialized, then you should be BUG'ing even
without this patch; the patch you've bisected to is only modifying the
*second* (or later) attempt to send the command, and it's using the same
'ec_dev' structure.

Furthermore, the only assignments to this 'proto_version' field look
like they're only writing one of 0, 2, 3, or

   min(EC_HOST_REQUEST_VERSION, fls(proto_info->protocol_versions) - 1)

. I don't see where 0xffff comes from.

So...is there any chance we've got a heap corruption somewhere?
Somebody's overwriting 'ec_dev->proto_version' accidentally?

Brian