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

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

[Dan Carpenter]

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>
---
 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,
-- 
2.20.1

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

[Jonathan Cameron]

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,

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

[Dan Carpenter]

On Sat, Sep 11, 2021 at 04:42:53PM +0100, Jonathan Cameron wrote:
> 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?
> 

Yeah.  This is from static analysis.  It's not a bug that probably
affects real life.

I guess it's debatable if it should get a Fixes tag, but I feel like
everything should be written in a hardened way.  Plus with the Fixes tag
it will get backported.

> 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.

That's true.  I will send that as a separate fix though.  Definitely
with a Fixes tag on that.  ;)

regards,
dan carpenter

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

[Jonathan Cameron]

On Mon, 13 Sep 2021 10:37:09 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Sat, Sep 11, 2021 at 04:42:53PM +0100, Jonathan Cameron wrote:
> > 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?
> >   
> 
> Yeah.  This is from static analysis.  It's not a bug that probably
> affects real life.
> 
> I guess it's debatable if it should get a Fixes tag, but I feel like
> everything should be written in a hardened way.  Plus with the Fixes tag
> it will get backported.
> 
> > 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.  
> 
> That's true.  I will send that as a separate fix though.  Definitely
> with a Fixes tag on that.  ;)

Ok. Applied to fixes-togreg branch of iio.igt and marked for stable.
Hardening this can't do us any harm as far as I can tell and it is a good thing
to do then it is sensible to backport it.

Thanks,

Jonathan

> 
> regards,
> dan carpenter
>