linux-kernel - Re: [PATCH] mfd: cros_ec: Add support for MKBP more event flags

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: <CA+ASDXPt5fws82r71xjuVucB=gZCYVSjx5dMaGdbW6qFOuG0vA@mail.gmail.com>
Date:   Mon, 14 Jan 2019 18:03:54 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     Gwendal Grignou <gwendal@...gle.com>
Cc:     Enrico Granata <egranata@...gle.com>,
        Lee Jones <lee.jones@...aro.org>,
        Benson Leung <bleung@...omium.org>,
        Olof Johansson <olof@...om.net>,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Enrico Granata <egranata@...omium.org>
Subject: Re: [PATCH] mfd: cros_ec: Add support for MKBP more event flags

Hi Gwendal,

On Mon, Jan 14, 2019 at 3:50 PM Gwendal Grignou <gwendal@...gle.com> wrote:
> On Fri, Dec 7, 2018 at 2:22 PM Brian Norris <briannorris@...omium.org> wrote:
> > On Thu, Nov 29, 2018 at 11:55:48AM -0800, egranata@...gle.com wrote:
> > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> > >       ret = cros_ec_get_host_command_version_mask(ec_dev,
> > >                                                   EC_CMD_GET_NEXT_EVENT,
> > >                                                   &ver_mask);
> >
> > It's not exactly new here (although you're using 'ver_mask' in new
> > ways), but cros_ec_get_host_command_version_mask() doesn't look 100%
> > right. It doesn't look at msg->result, and instead just assumes that if
> > we got some data back (send_command() > 0), then it must have been a
> > success. I don't think that's really guaranteed in general, although it
> > might be for the specific case of EC_CMD_GET_CMD_VERSIONS.

> It is guaranteed: if msg->result is not EC_RES_SUCCESS, then ret can
> not be greater than 0. At best it will be 0, or a negative number if
> we can already qualify the error in the errno space (see
> cros_ec_pkt_xfer_i2c() for instance).

Sorry, where do you guarantee that? The only enforcements I see in
those xfer implementation are:
(1) if result == EC_RES_IN_PROGRESS, we convert that to an errno
(2) if the expected length or checksum are bad, we turn that to an errno

So technically, the EC *could* be sending a valid, checksummed
response of the expected length, while still setting the ->result
field to something besides EC_RES_SUCCESS or EC_RES_IN_PROGRESS. And
we would treat that as a valid 'ver_mask'.

Albeit, that seems unlikely, given understanding of how the EC is
supposed to behave, but our code is not properly defensive AIUI. This
is basically why cros_ec_cmd_xfer_status() exists -- so that
sub-drivers don't get lazy and use cros_ec_cmd_xfer() without handling
the ->result field properly.

Brian