Re: [PATCH] iio: ssp_sensors: add more range checking in ssp_parse_dataframe()

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 9 Sep 2021 12:13:36 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> The "idx" is validated at the start of the loop but it gets incremented
> during the iteration so it needs to be checked again.
> 
> Fixes: 50dd64d57eee ("iio: common: ssp_sensors: Add sensorhub driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

This is only a fix if we assume that the len value check is there
as a protection against buffer overrun rather than as a termination condition
that occurs when parsing a valid record.

There is more paranoid checking in ssp_print_mc_debug() so it seems we aren't assuming
valid data in there at least.

Still is this perhaps a case of hardening rather than a fix or am I missing something?

As an aside, if that ssp_print_mcu_debug() reads a negative char it is then
returned directly so we get a random small negative number as the error code which
isn't going to be very useful.

Jonathan


> ---
>  drivers/iio/common/ssp_sensors/ssp_spi.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_spi.c b/drivers/iio/common/ssp_sensors/ssp_spi.c
> index 4864c38b8d1c..387551eac184 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_spi.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_spi.c
> @@ -273,6 +273,8 @@ static int ssp_parse_dataframe(struct ssp_data *data, char *dataframe, int len)
>  	for (idx = 0; idx < len;) {
>  		switch (dataframe[idx++]) {
>  		case SSP_MSG2AP_INST_BYPASS_DATA:
> +			if (idx >= len)
> +				return -EPROTO;
>  			sd = dataframe[idx++];
>  			if (sd < 0 || sd >= SSP_SENSOR_MAX) {
>  				dev_err(SSP_DEV,
> @@ -282,10 +284,13 @@ static int ssp_parse_dataframe(struct ssp_data *data, char *dataframe, int len)
>  
>  			if (indio_devs[sd]) {
>  				spd = iio_priv(indio_devs[sd]);
> -				if (spd->process_data)
> +				if (spd->process_data) {
> +					if (idx >= len)
> +						return -EPROTO;
>  					spd->process_data(indio_devs[sd],
>  							  &dataframe[idx],
>  							  data->timestamp);
> +				}
>  			} else {
>  				dev_err(SSP_DEV, "no client for frame\n");
>  			}
> @@ -293,6 +298,8 @@ static int ssp_parse_dataframe(struct ssp_data *data, char *dataframe, int len)
>  			idx += ssp_offset_map[sd];
>  			break;
>  		case SSP_MSG2AP_INST_DEBUG_DATA:
> +			if (idx >= len)
> +				return -EPROTO;
>  			sd = ssp_print_mcu_debug(dataframe, &idx, len);
>  			if (sd) {
>  				dev_err(SSP_DEV,