iio: chemical: scd30: avoid potential NULL deref in scd30_i2c_command()

[PATCH] iio: chemical: scd30: avoid potential NULL deref in scd30_i2c_command()

Posted by Stepan Ionichev 2 weeks, 4 days ago

scd30_i2c_command() takes an opaque "response" buffer plus its size.
At the start of the function the code already checks if response is
NULL (via the rsp local), but the response-decoding loop after the
i2c transfer always dereferences rsp without re-checking.

With the current callers in scd30_core.c this is harmless, since
write commands pass response=NULL together with size=0 (so the loop
body is never entered). However, the inconsistency is an accident
waiting to happen if a future caller passes response=NULL together
with size > 0 -- the loop would then write through a NULL pointer.

smatch flags this:

  drivers/iio/chemical/scd30_i2c.c:104 scd30_i2c_command() error: we
    previously assumed rsp could be null (see line 77)

Bail out early when rsp is NULL so the function is robust regardless
of the (cmd, size) combination chosen by the caller.

No functional change for the existing callers.

Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 drivers/iio/chemical/scd30_i2c.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/chemical/scd30_i2c.c b/drivers/iio/chemical/scd30_i2c.c
index 436df9c61..fb06bec75 100644
--- a/drivers/iio/chemical/scd30_i2c.c
+++ b/drivers/iio/chemical/scd30_i2c.c
@@ -93,6 +93,9 @@ static int scd30_i2c_command(struct scd30_state *state, enum scd30_cmd cmd, u16
 	if (ret)
 		return ret;
 
+	if (!rsp)
+		return 0;
+
 	/* validate received data and strip off crc bytes */
 	for (i = 0; i < size; i += 3) {
 		crc = crc8(scd30_i2c_crc8_tbl, buf + i, 2, CRC8_INIT_VALUE);
-- 
2.43.0

Re: [PATCH] iio: chemical: scd30: avoid potential NULL deref in scd30_i2c_command()

Posted by Jonathan Cameron 2 weeks, 4 days ago

On Wed,  6 May 2026 23:15:33 +0500
Stepan Ionichev <sozdayvek@gmail.com> wrote:

> scd30_i2c_command() takes an opaque "response" buffer plus its size.
> At the start of the function the code already checks if response is
> NULL (via the rsp local), but the response-decoding loop after the
> i2c transfer always dereferences rsp without re-checking.
> 
> With the current callers in scd30_core.c this is harmless, since
> write commands pass response=NULL together with size=0 (so the loop
> body is never entered). However, the inconsistency is an accident
> waiting to happen if a future caller passes response=NULL together
> with size > 0 -- the loop would then write through a NULL pointer.
> 
> smatch flags this:
> 
>   drivers/iio/chemical/scd30_i2c.c:104 scd30_i2c_command() error: we
>     previously assumed rsp could be null (see line 77)
> 
> Bail out early when rsp is NULL so the function is robust regardless
> of the (cmd, size) combination chosen by the caller.
> 
> No functional change for the existing callers.
> 
> Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
Hi Stephan

Thanks for the analysis - as you say no actual bug here but good
to make that more obvious to static analzers.

If we ever did hit this I think it would be better to return an
error code.  I'd also do it at the top given such a combination doesn't
make sense so we should exclude it early.

if (!response && size != 0)
	return -EINVAL;


> ---
>  drivers/iio/chemical/scd30_i2c.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iio/chemical/scd30_i2c.c b/drivers/iio/chemical/scd30_i2c.c
> index 436df9c61..fb06bec75 100644
> --- a/drivers/iio/chemical/scd30_i2c.c
> +++ b/drivers/iio/chemical/scd30_i2c.c
> @@ -93,6 +93,9 @@ static int scd30_i2c_command(struct scd30_state *state, enum scd30_cmd cmd, u16
>  	if (ret)
>  		return ret;
>  
> +	if (!rsp)
> +		return 0;
> +
>  	/* validate received data and strip off crc bytes */
>  	for (i = 0; i < size; i += 3) {
>  		crc = crc8(scd30_i2c_crc8_tbl, buf + i, 2, CRC8_INIT_VALUE);

[PATCH v2] iio: chemical: scd30: reject (response=NULL, size>0) in scd30_i2c_command()

Posted by Stepan Ionichev 2 weeks, 4 days ago

scd30_i2c_command() takes an opaque "response" buffer plus its size.
At the start of the function the code already checks if response is
NULL (via the rsp local), but the response-decoding loop after the
i2c transfer always dereferences rsp without re-checking. With the
current callers in scd30_core.c this is harmless, since write
commands pass response=NULL together with size=0 (so the loop body
is never entered).

The (response=NULL, size>0) combination has no useful meaning: there
is nowhere to put the bytes that come back from the chip. Treat it
as an invalid argument and bail out at the top of the function with
-EINVAL, instead of silently doing the i2c transfer and dereferencing
a NULL pointer in the decode loop.

smatch flagged the inconsistency:

  drivers/iio/chemical/scd30_i2c.c:104 scd30_i2c_command() error: we
    previously assumed rsp could be null (see line 77)

No functional change for the existing callers, which only ever use
(response=NULL, size=0) for writes and (response!=NULL, size>0) for
reads.

Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
v2:
- Move the check to the top of the function and return -EINVAL on
  the (response=NULL, size>0) combination, as suggested by Jonathan
  Cameron. Drop the v1 "if (!rsp) return 0" deeper in the function.

 drivers/iio/chemical/scd30_i2c.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iio/chemical/scd30_i2c.c b/drivers/iio/chemical/scd30_i2c.c
index 436df9c61..845c59a6b 100644
--- a/drivers/iio/chemical/scd30_i2c.c
+++ b/drivers/iio/chemical/scd30_i2c.c
@@ -71,6 +71,9 @@ static int scd30_i2c_command(struct scd30_state *state, enum scd30_cmd cmd, u16
 	int i, ret;
 	char crc;

+	if (!response && size != 0)
+		return -EINVAL;
+
 	put_unaligned_be16(scd30_i2c_cmd_lookup_tbl[cmd], buf);
 	i = 2;

--
2.43.0