[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@xxxxxxxxx>
---
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