linux-kernel - Re: [PATCH v6 01/11] platform: chrome: sensorhub: Add FIFO support

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPUE2uvZdbeU0zAgCGErDbDqu-VifVuNcLzvuo6mYY1MwYsaPQ@mail.gmail.com>
Date:   Thu, 26 Mar 2020 01:56:06 -0700
From:   Gwendal Grignou <gwendal@...omium.org>
To:     Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc:     Benson Leung <bleung@...omium.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 01/11] platform: chrome: sensorhub: Add FIFO support

On Wed, Mar 25, 2020 at 9:28 AM Enric Balletbo i Serra
<enric.balletbo@...labora.com> wrote:
>
> Hi Gwendal,
>
> Many thanks for sending this series upstream. Just one comment, other than that
> looks good to me.
>
> On 24/3/20 21:27, Gwendal Grignou wrote:
> > cros_ec_sensorhub registers a listener and query motion sense FIFO,
> > spread to iio sensors registers.
> >
> > To test, we can use libiio:
> > iiod&
> > iio_readdev -u ip:localhost -T 10000 -s 25 -b 16 cros-ec-gyro | od -x
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> > Signed-off-by: Gwendal Grignou <gwendal@...omium.org>
>
> [snip]
>
> > +/**
> > + * cros_ec_sensorhub_ring_handler() - The trigger handler function
> > + *
> > + * @sensorhub: Sensor Hub object.
> > + *
> > + * Called by the notifier, process the EC sensor FIFO queue.
> > + */
> > +static void cros_ec_sensorhub_ring_handler(struct cros_ec_sensorhub *sensorhub)
> > +{
> > +     struct cros_ec_fifo_info *fifo_info = &sensorhub->fifo_info;
> > +     struct cros_ec_dev *ec = sensorhub->ec;
> > +     ktime_t fifo_timestamp, current_timestamp;
> > +     int i, j, number_data, ret;
> > +     unsigned long sensor_mask = 0;
> > +     struct ec_response_motion_sensor_data *in;
> > +     struct cros_ec_sensors_ring_sample *out, *last_out;
> > +
> > +     mutex_lock(&sensorhub->cmd_lock);
> > +
> > +     /* Get FIFO information if there are lost vectors. */
> > +     if (fifo_info->info.total_lost) {
> > +             /* Need to retrieve the number of lost vectors per sensor */
> > +             sensorhub->params->cmd = MOTIONSENSE_CMD_FIFO_INFO;
> > +             sensorhub->msg->outsize = 1;
> > +             sensorhub->msg->insize =
> > +                     sizeof(struct ec_response_motion_sense_fifo_info) +
> > +                     sizeof(u16) * CROS_EC_SENSOR_MAX;
> > +
> > +             if (cros_ec_cmd_xfer_status(ec->ec_dev, sensorhub->msg) < 0)
> > +                     goto error;
> > +
> > +             memcpy(fifo_info, &sensorhub->resp->fifo_info,
> > +                    sizeof(*fifo_info));
> > +
>
> Smatch is reporting:
Which version of smatch are you using? I am using
v0.5.0-6279-g2f013029 and the problem is not reported.
>
> cros_ec_sensorhub_ring_handler() error: memcpy() '&sensorhub->resp->fifo_info'
> too small (10 vs 42)
>
> Is it fine and safe to copy always the 42 bytes? I suspect that we should only
> copy the number of lost events, total_lost , not always the maximum (16). Or the
> EC is always sending the full array (16 bytes)?
EC will not fill the 42 bytes if it has less than 16 sensors. It is
safe because we are not looking at the bytes we don't need, but it is
not clean.
Working on a new patch set where I remove the CROS_EC_SENSOR_MAX
constant and dynamically allocate the data I need based on the number
of sensors.
>
> Thanks,
> Enric
>