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

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

[Stepan Ionichev]

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

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

[Stepan Ionichev]

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

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

[Andy Shevchenko]

On Thu, May 07, 2026 at 08:28:00PM +0500, Stepan Ionichev 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).
> 
> 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.

Is this analysis AI assisted?

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

Do not reply to the same email thread with a new patch version.

...

Code wise LGTM, thanks.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

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

[Stepan Ionichev]

On Fri, 08 May 2026, Andy Shevchenko wrote:
> Is this analysis AI assisted?

The bug itself was found by smatch and analysed manually; AI
was used to help draft the commit message wording.

> Do not reply to the same email thread with a new patch version.

Got it -- will send future v-N in separate threads.

Thanks for the review!

Stepan

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

[Maxwell Doose]

On Thu May 7, 2026 at 10:28 AM CDT, Stepan Ionichev 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).
>
[snip]
> @@ -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;
>

I guess we're still handling the cases where both response/rsp and
size are zero here below?

	if (rsp) {
		/* each two bytes are followed by a crc8 */
		size += size / 2;
	} else {
		put_unaligned_be16(arg, buf + i);
		crc = crc8(scd30_i2c_crc8_tbl, buf + i, 2, CRC8_INIT_VALUE);
		i += 2;
		buf[i] = crc;
		i += 1;

		/* commands below don't take an argument */
		if ((cmd == CMD_STOP_MEAS) || (cmd == CMD_RESET))
			i -= 3;
	}

Should add a comment showing that this handles this case but that's
just my personal nit. Not worth forcing a v3 so

Acked-by: Maxwell Doose <m32285159@gmail.com>

best regards,
max

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

[Stepan Ionichev]

On Fri, 08 May 2026, Maxwell Doose wrote:
> I guess we're still handling the cases where both response/rsp and
> size are zero here below?
>
> Should add a comment showing that this handles this case but that's
> just my personal nit. Not worth forcing a v3 so
>
> Acked-by: Maxwell Doose <m32285159@gmail.com>

Thanks for the ack and the review!

You're right -- the (NULL, 0) write-command path is still handled
correctly by the existing rsp-NULL branch below; the new check only
rejects the genuinely buggy (NULL, size>0) combination. Happy to add
a short comment making that intent explicit if Jonathan would prefer
a v3, otherwise I'll keep it noted for a future cleanup.

Stepan

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

[Maxwell Doose]

On Fri, May 8, 2026 at 2:16 PM Stepan Ionichev <sozdayvek@gmail.com> wrote:
>
> On Fri, 08 May 2026, Maxwell Doose wrote:
> > I guess we're still handling the cases where both response/rsp and
> > size are zero here below?
> >
> > Should add a comment showing that this handles this case but that's
> > just my personal nit. Not worth forcing a v3 so
> >
> > Acked-by: Maxwell Doose <m32285159@gmail.com>
>
> Thanks for the ack and the review!
>
> You're right -- the (NULL, 0) write-command path is still handled
> correctly by the existing rsp-NULL branch below; the new check only
> rejects the genuinely buggy (NULL, size>0) combination. Happy to add
> a short comment making that intent explicit if Jonathan would prefer
> a v3, otherwise I'll keep it noted for a future cleanup.
>

BTW, if Jonathan asks you to resend for some other technical issue,
please add that comment. Otherwise if he doesn't ask for v3 or doesn't
include it himself, don't bother. Also, keep in mind that "Acked-by !=
Reviewed-by". Just means the patch is appropriate for inclusion.

best regards,
max

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

[Jonathan Cameron]

On Fri, 8 May 2026 14:50:24 -0500
Maxwell Doose <m32285159@gmail.com> wrote:

> On Fri, May 8, 2026 at 2:16 PM Stepan Ionichev <sozdayvek@gmail.com> wrote:
> >
> > On Fri, 08 May 2026, Maxwell Doose wrote:  
> > > I guess we're still handling the cases where both response/rsp and
> > > size are zero here below?
> > >
> > > Should add a comment showing that this handles this case but that's
> > > just my personal nit. Not worth forcing a v3 so
> > >
> > > Acked-by: Maxwell Doose <m32285159@gmail.com>  
> >
> > Thanks for the ack and the review!
> >
> > You're right -- the (NULL, 0) write-command path is still handled
> > correctly by the existing rsp-NULL branch below; the new check only
> > rejects the genuinely buggy (NULL, size>0) combination. Happy to add
> > a short comment making that intent explicit if Jonathan would prefer
> > a v3, otherwise I'll keep it noted for a future cleanup.
> >  
> 
> BTW, if Jonathan asks you to resend for some other technical issue,
> please add that comment. Otherwise if he doesn't ask for v3 or doesn't
> include it himself, don't bother. Also, keep in mind that "Acked-by !=
> Reviewed-by". Just means the patch is appropriate for inclusion.

In my view it is a little borderline on whether such a comment
is useful given the fairly obvious && rather than || in the check
added. Anyhow, no other reason to respin to I've applied this as is.

Thanks,

Jonathan

> 
> best regards,
> max

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

[Jonathan Cameron]

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);