| 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
| ||
|
Message-ID: <0af8a4bc994b4e90cb0d079d1c7f105dd2a60e32.camel@collabora.com>
Date: Fri, 28 Jun 2019 13:37:07 +0200
From: Fabien Lahoudere <fabien.lahoudere@...labora.com>
To: Gwendal Grignou <gwendal@...omium.org>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc: kernel@...labora.com, Nick Vaccaro <nvaccaro@...omium.org>,
Jonathan Cameron <jic23@...nel.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald-Stadler <pmeerw@...erw.net>,
linux-iio <linux-iio@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Doug Anderson <dianders@...omium.org>,
Enrico Granata <egranata@...omium.org>
Subject: Re: [PATCH 1/2] iio: common: cros_ec_sensors: determine protocol
version
Le jeudi 27 juin 2019 à 14:59 -0700, Gwendal Grignou a écrit :
> On Thu, Jun 27, 2019 at 8:59 AM Enric Balletbo i Serra
> <enric.balletbo@...labora.com> wrote:
> > Hi,
> >
> > cc'ing Doug, Gwendal and Enrico that might be interested to give a
> > review.
> >
> > This patch can be picked alone without 2/2, an is needed to have
> > cros-ec-sensors
> > legacy support on ARM (see [1] and [2])
> >
> > Jonathan, as [1] and [2] will go through the chrome-platform tree
> > if you don't
> > mind I'd also like to carry with this patch once you're fine with
> > it.
> >
> > Thanks,
> > ~ Enric
> >
> > [1] https://patchwork.kernel.org/patch/11014329/
> > [2] https://patchwork.kernel.org/patch/11014327/
> >
> > On 27/6/19 16:04, Fabien Lahoudere wrote:
> > > This patch adds a function to determine which version of the
> > > protocol is used to communicate with EC.
> > >
> > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@...labora.com>
> > > Signed-off-by: Nick Vaccaro <nvaccaro@...omium.org>
> >
> > Tested-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> >
> > > ---
> > > .../cros_ec_sensors/cros_ec_sensors_core.c | 36
> > > ++++++++++++++++++-
> > > 1 file changed, 35 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > index 130362ca421b..2e0f97448e64 100644
> > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > @@ -25,6 +25,31 @@ static char *cros_ec_loc[] = {
> > > [MOTIONSENSE_LOC_MAX] = "unknown",
> > > };
> > >
> > > +static int cros_ec_get_host_cmd_version_mask(struct
> > > cros_ec_device *ec_dev,
> > > + u16 cmd_offset, u16
> > > cmd, u32 *mask)
> > > +{
> > > + int ret;
> > > + struct {
> > > + struct cros_ec_command msg;
> > > + union {
> > > + struct ec_params_get_cmd_versions params;
> > > + struct ec_response_get_cmd_versions resp;
> > > + };
> > > + } __packed buf = {
> > > + .msg = {
> add
> .version = 0,
> As the variable is coming from the stack, the version should be set.
> > > + .command = EC_CMD_GET_CMD_VERSIONS +
> > > cmd_offset,
> > > + .insize = sizeof(struct
> > > ec_response_get_cmd_versions),
> > > + .outsize = sizeof(struct
> > > ec_params_get_cmd_versions)
> > > + },
> > > + .params = {.cmd = cmd}
> > > + };
> > > +
> > > + ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > > + if (ret >= 0)
> It should be > 0: when the command is a success, it returns the
> number
> of byte in the response buffer. When don't expect == 0 here, because
> when successful, EC_CMD_GET_CMD_VERSIONS will return a mask.
> > > + *mask = buf.resp.version_mask;
> > > + return ret;
> > > +}
> > > +
> > > int cros_ec_sensors_core_init(struct platform_device *pdev,
> > > struct iio_dev *indio_dev,
> > > bool physical_device)
> > > @@ -33,6 +58,8 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > > struct cros_ec_sensors_core_state *state =
> > > iio_priv(indio_dev);
> > > struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
> > > struct cros_ec_sensor_platform *sensor_platform =
> > > dev_get_platdata(dev);
> > > + u32 ver_mask;
> > > + int ret;
> > >
> > > platform_set_drvdata(pdev, indio_dev);
> > >
> > > @@ -47,8 +74,15 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > >
> > > mutex_init(&state->cmd_lock);
> > >
> > > + ret = cros_ec_get_host_cmd_version_mask(state->ec,
> > > + ec->cmd_offset,
> > > + EC_CMD_MOTION_SENSE
> > > _CMD,
> > > + &ver_mask);
> > > + if (ret < 0)
> Use:
> if (ret <= 0 || ver_mask == 0) {
> In case the EC is really old or misbehaving, we don't want to set an
> invalid version later.
To not return a positive value on error if ret >= 0 and ver_mask = 0
I would prefer this:
if (ret <= 0)
return ret;
if (ver_mask == 0)
return -EIO;
Let me know if I am wrong
> > > + return ret;
> > > +
> > > /* Set up the host command structure. */
> > > - state->msg->version = 2;
> > > + state->msg->version = fls(ver_mask) - 1;;
> > > state->msg->command = EC_CMD_MOTION_SENSE_CMD + ec-
> > > >cmd_offset;
> > > state->msg->outsize = sizeof(struct
> > > ec_params_motion_sense);
> > >
> > >
Powered by blists - more mailing lists