Re: [bug report] iio: common: ssp_sensors: Add sensorhub driver

[Dan Carpenter <dan.carpenter@oracle.com>]

On Sat, Apr 24, 2021 at 01:12:46AM +0300, Andy Shevchenko wrote:
> On Friday, April 23, 2021, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > Hello Karol Wrona,
> >
> > The patch 50dd64d57eee: "iio: common: ssp_sensors: Add sensorhub
> > driver" from Jan 28, 2015, leads to the following static checker
> > warning:
> >
> >         drivers/iio/common/ssp_sensors/ssp_spi.c:276 ssp_parse_dataframe()
> >         warn: check that incremented offset 'idx' is capped
> >
> > drivers/iio/common/ssp_sensors/ssp_spi.c
> >    267  static int ssp_parse_dataframe(struct ssp_data *data, char
> > *dataframe, int len)
> >    268  {
> >    269          int idx, sd;
> >    270          struct ssp_sensor_data *spd;
> >    271          struct iio_dev **indio_devs = data->sensor_devs;
> >    272
> >    273          for (idx = 0; idx < len;) {
> >    274                  switch (dataframe[idx++]) {
> >    275                  case SSP_MSG2AP_INST_BYPASS_DATA:
> >
> > There needs to be be a check here:
> >
> >                                 if (idx == len)
> >                                         return -EPROTO;
> >
> >    276                          sd = dataframe[idx++];
> >    277                          if (sd < 0 || sd >= SSP_SENSOR_MAX) {
> >    278                                  dev_err(SSP_DEV,
> >    279                                          "Mcu data frame1 error
> > %d\n", sd);
> >    280                                  return -EPROTO;
> >    281                          }
> >    282
> >    283                          if (indio_devs[sd]) {
> >    284                                  spd = iio_priv(indio_devs[sd]);
> >    285                                  if (spd->process_data)
> >    286
> > spd->process_data(indio_devs[sd],
> >    287
> > &dataframe[idx],
> >    288
> > data->timestamp);
> >
> > But then the problem is we don't pass the "len" to ->process_data().  I
> > looked at trying to fix this but it's a bit involved.
> 
> 
> Because infinite loops are bad. That’s why I try convince people to avoid
> them as much as possible.
> 

I'm not sure what you mean at all.  The issue here is that
->process_data() reads an unknown number of bytes.  We need to pass
"len - idx" to ->process_data() to detect the overflow and return -EPROTO.

regards,
dan carpenter

> 
> >
> >    289                          } else {
> >    290                                  dev_err(SSP_DEV, "no client for
> > frame\n");
> >    291                          }
> >    292
> >    293                          idx += ssp_offset_map[sd];
> >    294                          break;
> >    295                  case SSP_MSG2AP_INST_DEBUG_DATA:
> >    296                          sd = ssp_print_mcu_debug(dataframe, &idx,
> > len);
> >                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > There is another potential read overflow here because we read one more
> > byte before we check the "len".
> >
> >    297                          if (sd) {
> >    298                                  dev_err(SSP_DEV,
> >    299                                          "Mcu data frame3 error
> > %d\n", sd);
> >    300                                  return sd;
> >    301                          }
> >    302                          break;
> >    303                  case SSP_MSG2AP_INST_LIBRARY_DATA:
> >    304                          idx += len;
> >    305                          break;
> >    306                  case SSP_MSG2AP_INST_BIG_DATA:
> >    307                          ssp_handle_big_data(data, dataframe, &idx);
> >    308                          break;
> >    309                  case SSP_MSG2AP_INST_TIME_SYNC:
> >    310                          data->time_syncing = true;
> >    311                          break;
> >    312                  case SSP_MSG2AP_INST_RESET:
> >    313                          ssp_queue_ssp_refresh_task(data, 0);
> >    314                          break;
> >    315                  }
> >    316          }
> >    317
> >    318          if (data->time_syncing)
> >    319                  data->timestamp = ktime_get_real_ns();
> >    320
> >    321          return 0;
> >    322  }
> >
> > regards,
> > dan carpenter
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko