iio: chemical: sps30: Add Null pointer check

[PATCH] iio: chemical: sps30: Add Null pointer check

Posted by Karan Sanghavi 1 month, 1 week ago

Add a Null pointer check before assigning and incrementing
the null pointer

Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
---
 drivers/iio/chemical/sps30_i2c.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/chemical/sps30_i2c.c b/drivers/iio/chemical/sps30_i2c.c
index 1b21b6bcd0e7..d2142e4c748c 100644
--- a/drivers/iio/chemical/sps30_i2c.c
+++ b/drivers/iio/chemical/sps30_i2c.c
@@ -105,16 +105,18 @@ static int sps30_i2c_command(struct sps30_state *state, u16 cmd, void *arg, size
 		return ret;
 
 	/* validate received data and strip off crc bytes */
-	tmp = rsp;
-	for (i = 0; i < rsp_size; i += 3) {
-		crc = crc8(sps30_i2c_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
-		if (crc != buf[i + 2]) {
-			dev_err(state->dev, "data integrity check failed\n");
-			return -EIO;
+	if (rsp) {
+		tmp = rsp;
+		for (i = 0; i < rsp_size; i += 3) {
+			crc = crc8(sps30_i2c_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
+			if (crc != buf[i + 2]) {
+				dev_err(state->dev, "data integrity check failed\n");
+				return -EIO;
+			}
+
+			*tmp++ = buf[i];
+			*tmp++ = buf[i + 1];
 		}
-
-		*tmp++ = buf[i];
-		*tmp++ = buf[i + 1];
 	}
 
 	return 0;

---
base-commit: f2493655d2d3d5c6958ed996b043c821c23ae8d3
change-id: 20241018-cid1593398badshift-9c512a3b92d9

Best regards,
-- 
Karan Sanghavi <karansanghvi98@gmail.com>

Re: [PATCH] iio: chemical: sps30: Add Null pointer check

Posted by Jonathan Cameron 1 month, 1 week ago

On Fri, 18 Oct 2024 18:54:42 +0000
Karan Sanghavi <karansanghvi98@gmail.com> wrote:

> Add a Null pointer check before assigning and incrementing
> the null pointer
> 
> Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>

It would be a bug if rsp_size was anything other than 0 and rsp is NULL.
So this looks like a false positive as the loop will never be
entered.

How did you find it, in particular have you managed to trigger this
in the driver?

Jonathan


> ---
>  drivers/iio/chemical/sps30_i2c.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/chemical/sps30_i2c.c b/drivers/iio/chemical/sps30_i2c.c
> index 1b21b6bcd0e7..d2142e4c748c 100644
> --- a/drivers/iio/chemical/sps30_i2c.c
> +++ b/drivers/iio/chemical/sps30_i2c.c
> @@ -105,16 +105,18 @@ static int sps30_i2c_command(struct sps30_state *state, u16 cmd, void *arg, size
>  		return ret;
>  
>  	/* validate received data and strip off crc bytes */
> -	tmp = rsp;
> -	for (i = 0; i < rsp_size; i += 3) {
> -		crc = crc8(sps30_i2c_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
> -		if (crc != buf[i + 2]) {
> -			dev_err(state->dev, "data integrity check failed\n");
> -			return -EIO;
> +	if (rsp) {
> +		tmp = rsp;
> +		for (i = 0; i < rsp_size; i += 3) {
> +			crc = crc8(sps30_i2c_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
> +			if (crc != buf[i + 2]) {
> +				dev_err(state->dev, "data integrity check failed\n");
> +				return -EIO;
> +			}
> +
> +			*tmp++ = buf[i];
> +			*tmp++ = buf[i + 1];
>  		}
> -
> -		*tmp++ = buf[i];
> -		*tmp++ = buf[i + 1];
>  	}
>  
>  	return 0;
> 
> ---
> base-commit: f2493655d2d3d5c6958ed996b043c821c23ae8d3
> change-id: 20241018-cid1593398badshift-9c512a3b92d9
> 
> Best regards,

Re: [PATCH] iio: chemical: sps30: Add Null pointer check

Posted by Karan Sanghavi 1 month, 1 week ago

On Sat, Oct 19, 2024 at 12:21:33PM +0100, Jonathan Cameron wrote:
> On Fri, 18 Oct 2024 18:54:42 +0000
> Karan Sanghavi <karansanghvi98@gmail.com> wrote:
> 
> > Add a Null pointer check before assigning and incrementing
> > the null pointer
> > 
> > Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
> 
> It would be a bug if rsp_size was anything other than 0 and rsp is NULL.
> So this looks like a false positive as the loop will never be
> entered.
> 
> How did you find it, in particular have you managed to trigger this
> in the driver?
> 
> Jonathan
> 
>

I found this bug in Coverity scan with Cid: 1504707.
Link below, for the same.
https://scan7.scan.coverity.com/#/project-view/51946/11354?selectedIssue=1504707

Rsp here is a void pointer received from the function arguments 
which can be NULL for a no respone call.
Thus incrementing the NULL pointer can lead to some unexpected 
behavior which cross my mind thus added the check. 


> > ---
> >  drivers/iio/chemical/sps30_i2c.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/sps30_i2c.c b/drivers/iio/chemical/sps30_i2c.c
> > index 1b21b6bcd0e7..d2142e4c748c 100644
> > --- a/drivers/iio/chemical/sps30_i2c.c
> > +++ b/drivers/iio/chemical/sps30_i2c.c
> > @@ -105,16 +105,18 @@ static int sps30_i2c_command(struct sps30_state *state, u16 cmd, void *arg, size
> >  		return ret;
> >  
> >  	/* validate received data and strip off crc bytes */
> > -	tmp = rsp;
> > -	for (i = 0; i < rsp_size; i += 3) {
> > -		crc = crc8(sps30_i2c_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
> > -		if (crc != buf[i + 2]) {
> > -			dev_err(state->dev, "data integrity check failed\n");
> > -			return -EIO;
> > +	if (rsp) {
> > +		tmp = rsp;
> > +		for (i = 0; i < rsp_size; i += 3) {
> > +			crc = crc8(sps30_i2c_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
> > +			if (crc != buf[i + 2]) {
> > +				dev_err(state->dev, "data integrity check failed\n");
> > +				return -EIO;
> > +			}
> > +
> > +			*tmp++ = buf[i];
> > +			*tmp++ = buf[i + 1];
> >  		}
> > -
> > -		*tmp++ = buf[i];
> > -		*tmp++ = buf[i + 1];
> >  	}
> >  
> >  	return 0;
> > 
> > ---
> > base-commit: f2493655d2d3d5c6958ed996b043c821c23ae8d3
> > change-id: 20241018-cid1593398badshift-9c512a3b92d9
> > 
> > Best regards,
>
Thank you,
Karan.

Re: [PATCH] iio: chemical: sps30: Add Null pointer check

Posted by Shuah Khan 1 month ago

On 10/19/24 06:08, Karan Sanghavi wrote:
> On Sat, Oct 19, 2024 at 12:21:33PM +0100, Jonathan Cameron wrote:
>> On Fri, 18 Oct 2024 18:54:42 +0000
>> Karan Sanghavi <karansanghvi98@gmail.com> wrote:
>>
>>> Add a Null pointer check before assigning and incrementing
>>> the null pointer
>>>
>>> Signed-off-by: Karan Sanghavi <karansanghvi98@gmail.com>
>>
>> It would be a bug if rsp_size was anything other than 0 and rsp is NULL.
>> So this looks like a false positive as the loop will never be
>> entered.
>>

This routine checks rsp in the earlier logic

	if (rsp) {
                 /* each two bytes are followed by a crc8 */
                 rsp_size += rsp_size / 2;
         } else {
                 tmp = arg;

                 while (arg_size) {
                         buf[i] = *tmp++;
                         buf[i + 1] = *tmp++;
                         buf[i + 2] = crc8(sps30_i2c_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
                         arg_size -= 2;
                         i += 3;
                 }
         }
  	
	ret = sps30_i2c_xfer(state, buf, i, buf, rsp_size);
         if (ret)
                 return ret;


Looks like the  tmp = rsp; could be reached depending on the
sps30_i2c_xfer() return value?

Maybe this isn't the right fix but looks like the code could
use looking into for accuracy.

>> How did you find it, in particular have you managed to trigger this
>> in the driver?
>>
>> Jonathan
>>
>>
> 
> I found this bug in Coverity scan with Cid: 1504707.
> Link below, for the same.
> https://scan7.scan.coverity.com/#/project-view/51946/11354?selectedIssue=1504707
> 
> Rsp here is a void pointer received from the function arguments
> which can be NULL for a no respone call.
> Thus incrementing the NULL pointer can lead to some unexpected
> behavior which cross my mind thus added the check.
> 

thanks,
-- Shuah