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

[Shuah Khan]

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@xxxxxxxxx> wrote:
> >
> > > Add a Null pointer check before assigning and incrementing
> > > the null pointer
> > >
> > > Signed-off-by: Karan Sanghavi <karansanghvi98@xxxxxxxxx>
> >
> > 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