[PATCH] firmware: arm_scmi: Fix late checks on pointer dereference

[PATCH] firmware: arm_scmi: Fix late checks on pointer dereference

[Cristian Marussi]

A few dereferences could happen before the iterator pointer argument was
checked for NULL, causing the following smatch warnings:

drivers/firmware/arm_scmi/driver.c:1214 scmi_iterator_run() warn: variable
dereferenced before check 'i' (see line 1210)

Fix by moving the checks early and dropping some unneeded local references.

No functional change.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index c4960fd3df75..c1922bd650ae 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1205,18 +1205,21 @@ static void *scmi_iterator_init(const struct scmi_protocol_handle *ph,
 static int scmi_iterator_run(void *iter)
 {
 	int ret = -EINVAL;
+	struct scmi_iterator_ops *iops;
+	const struct scmi_protocol_handle *ph;
+	struct scmi_iterator_state *st;
 	struct scmi_iterator *i = iter;
-	struct scmi_iterator_state *st = &i->state;
-	struct scmi_iterator_ops *iops = i->ops;
-	const struct scmi_protocol_handle *ph = i->ph;
-	const struct scmi_xfer_ops *xops = ph->xops;
 
-	if (!i)
+	if (!i || !i->ops || !i->ph)
 		return ret;
 
+	iops = i->ops;
+	ph = i->ph;
+	st = &i->state;
+
 	do {
 		iops->prepare_message(i->msg, st->desc_index, i->priv);
-		ret = xops->do_xfer(ph, i->t);
+		ret = ph->xops->do_xfer(ph, i->t);
 		if (ret)
 			break;
 
@@ -1240,7 +1243,7 @@ static int scmi_iterator_run(void *iter)
 		}
 
 		st->desc_index += st->num_returned;
-		xops->reset_rx_to_maxsz(ph, i->t);
+		ph->xops->reset_rx_to_maxsz(ph, i->t);
 		/*
 		 * check for both returned and remaining to avoid infinite
 		 * loop due to buggy firmware
@@ -1249,7 +1252,7 @@ static int scmi_iterator_run(void *iter)
 
 out:
 	/* Finalize and destroy iterator */
-	xops->xfer_put(ph, i->t);
+	ph->xops->xfer_put(ph, i->t);
 	devm_kfree(ph->dev, i);
 
 	return ret;
-- 
2.32.0

Re: [PATCH] firmware: arm_scmi: Fix late checks on pointer dereference

[Dan Carpenter]

On Tue, May 03, 2022 at 01:10:47PM +0100, Cristian Marussi wrote:
> A few dereferences could happen before the iterator pointer argument was
> checked for NULL, causing the following smatch warnings:
> 
> drivers/firmware/arm_scmi/driver.c:1214 scmi_iterator_run() warn: variable
> dereferenced before check 'i' (see line 1210)
> 
> Fix by moving the checks early and dropping some unneeded local references.
> 
> No functional change.

If there is no chance these can be NULL then a different option is to
just delete the NULL checks.

regards,
dan carpenter

Re: [PATCH] firmware: arm_scmi: Fix late checks on pointer dereference

[Cristian Marussi]

On Wed, May 04, 2022 at 09:44:36AM +0300, Dan Carpenter wrote:
> On Tue, May 03, 2022 at 01:10:47PM +0100, Cristian Marussi wrote:
> > A few dereferences could happen before the iterator pointer argument was
> > checked for NULL, causing the following smatch warnings:
> > 
> > drivers/firmware/arm_scmi/driver.c:1214 scmi_iterator_run() warn: variable
> > dereferenced before check 'i' (see line 1210)
> > 
> > Fix by moving the checks early and dropping some unneeded local references.
> > 
> > No functional change.
> 

Hi Dan,

thanks for you feedback first of all.

> If there is no chance these can be NULL then a different option is to
> just delete the NULL checks.

Indeed, I think I kept only the checks on iter param that can possibly
be NULL if the caller messes up the usage of this iterator interface
(or if the internals are messed up by future developments...just to
play on the safe side).

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: Fix late checks on pointer dereference

[Sudeep Holla]

On Tue, 3 May 2022 13:10:47 +0100, Cristian Marussi wrote:
> A few dereferences could happen before the iterator pointer argument was
> checked for NULL, causing the following smatch warnings:
> 
> drivers/firmware/arm_scmi/driver.c:1214 scmi_iterator_run() warn: variable
> dereferenced before check 'i' (see line 1210)
> 
> Fix by moving the checks early and dropping some unneeded local references.
> 
> [...]

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/1] firmware: arm_scmi: Fix late checks on pointer dereference
      https://git.kernel.org/sudeep.holla/c/c7f8852d42

--
Regards,
Sudeep