* [PATCH] iio: ssp_sensors: add more range checking in ssp_parse_dataframe()
@ 2021-09-09 9:13 Dan Carpenter
2021-09-11 15:42 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-09-09 9:13 UTC (permalink / raw)
To: Jonathan Cameron, Karol Wrona
Cc: Lars-Peter Clausen, Kyungmin Park, linux-iio, kernel-janitors
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: ssp_sensors: add more range checking in ssp_parse_dataframe()
2021-09-09 9:13 [PATCH] iio: ssp_sensors: add more range checking in ssp_parse_dataframe() Dan Carpenter
@ 2021-09-11 15:42 ` Jonathan Cameron
2021-09-13 7:37 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2021-09-11 15:42 UTC (permalink / raw)
To: Dan Carpenter
Cc: Karol Wrona, Lars-Peter Clausen, Kyungmin Park, linux-iio,
kernel-janitors
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,
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: ssp_sensors: add more range checking in ssp_parse_dataframe()
2021-09-11 15:42 ` Jonathan Cameron
@ 2021-09-13 7:37 ` Dan Carpenter
2021-09-18 18:12 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-09-13 7:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Karol Wrona, Lars-Peter Clausen, Kyungmin Park, linux-iio,
kernel-janitors
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: ssp_sensors: add more range checking in ssp_parse_dataframe()
2021-09-13 7:37 ` Dan Carpenter
@ 2021-09-18 18:12 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2021-09-18 18:12 UTC (permalink / raw)
To: Dan Carpenter
Cc: Karol Wrona, Lars-Peter Clausen, Kyungmin Park, linux-iio,
kernel-janitors
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-18 18:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-09 9:13 [PATCH] iio: ssp_sensors: add more range checking in ssp_parse_dataframe() Dan Carpenter
2021-09-11 15:42 ` Jonathan Cameron
2021-09-13 7:37 ` Dan Carpenter
2021-09-18 18:12 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).