[PATCH] staging: greybus: bootrom: fix potential NULL dereference

[PATCH] staging: greybus: bootrom: fix potential NULL dereference

[hanzhijian]

In gb_bootrom_get_firmware(), the queue_work label dereferences fw->size
on a path where fw may have been set to NULL via the "if (!fw) goto
unlock" path.  This is currently masked at runtime by the !ret
short-circuit (ret is non-zero on every path where fw can be NULL), but
it relies on an implicit invariant that is fragile and hard to follow.

Move the next_request calculation into the mutex-protected region where
fw is known to be non-NULL, and initialize next_request to
NEXT_REQ_GET_FIRMWARE so the queue_work label no longer dereferences fw.

Signed-off-by: hanzhijian <hanzhijian1991@gmail.com>
---
 drivers/staging/greybus/bootrom.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
index 83921d90c..289c9e957 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -245,7 +245,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
 	struct gb_bootrom_get_firmware_request *firmware_request;
 	struct device *dev = &op->connection->bundle->dev;
 	unsigned int offset, size;
-	enum next_request_type next_request;
+	enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
 	u8 *firmware_response;
 	int ret = 0;
 
@@ -293,16 +293,14 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
 	dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n",
 		offset, size);
 
+	if (offset + size == fw->size)
+		next_request = NEXT_REQ_READY_TO_BOOT;
+
 unlock:
 	mutex_unlock(&bootrom->mutex);
 
 queue_work:
 	/* Refresh timeout */
-	if (!ret && (offset + size == fw->size))
-		next_request = NEXT_REQ_READY_TO_BOOT;
-	else
-		next_request = NEXT_REQ_GET_FIRMWARE;
-
 	gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
 
 	return ret;
-- 
2.43.0

Re: [PATCH] staging: greybus: bootrom: fix potential NULL dereference

[Dan Carpenter]

You're using the word "potential" but the commit message correctly
explains why a NULL dereference is impossible.  Don't say potentially
for things which are impossible.

On Fri, Aug 21, 2026 at 07:35:40PM +0800, hanzhijian wrote:
> In gb_bootrom_get_firmware(), the queue_work label dereferences fw->size
> on a path where fw may have been set to NULL via the "if (!fw) goto
> unlock" path.  This is currently masked at runtime by the !ret
> short-circuit (ret is non-zero on every path where fw can be NULL), but
> it relies on an implicit invariant that is fragile and hard to follow.

A lot of people would argue that the original code is easy to follow.  In
your code, to see what is passed on error you have to scroll all the way
to the top of the function to see the "next_request =
NEXT_REQ_GET_FIRMWARE;" assignment.  In the existing code, it's clear,
this is what we pass on error, this is what we pass on success.

It's not really fragile either.  If we screwed up and forgot to set the
error code or something then Smatch would warn about that.

drivers/staging/greybus/bootrom.c:300 gb_bootrom_get_firmware() error: we previously assumed 'fw' could be null (see line 266)

Or on the earlier paths, we would get an uninitialized variable
warning.

regards,
dan carpenter

Re: [PATCH] staging: greybus: bootrom: fix potential NULL dereference

[hanzhijian]

On Fri, Aug 21, 2026 at 02:53:13PM +0300, Dan Carpenter wrote:
> You're using the word "potential" but the commit message correctly
> explains why a NULL dereference is impossible.  Don't say potentially
> for things which are impossible.
>
> A lot of people would argue that the original code is easy to follow...
> It's not really fragile either...

You're right on all counts.  The commit message contradicts itself, and
the refactor does make the error path harder to follow than the original
if/else.  I'll drop this patch.

Thanks for the review.

hanzhijian