On Tue, Jun 21, 2022 at 10:36:04PM +0800, Dongliang Mu wrote:

Hi maintainers,

I would like to send one bug report.

In gb_bootrom_get_firmware, if the first branch is satisfied, it will go to queue_work, leading to the dereference of uninitialized const variable "fw". If the second branch is satisfied, it will go to unlock with fw as NULL pointer, leading to a NULL Pointer Dereference.

This sounds like the false positive that checkers keep tripping over.

Please double check your analysis and search the archives first.

Johan

On Tue, Jun 21, 2022 at 10:36:04PM +0800, Dongliang Mu wrote:

Hi maintainers,

I would like to send one bug report.

In gb_bootrom_get_firmware, if the first branch is satisfied, it will go to queue_work, leading to the dereference of uninitialized const variable "fw". If the second branch is satisfied, it will go to unlock with fw as NULL pointer, leading to a NULL Pointer Dereference.

The Fixes commit should be [1], introducing the dereference of "fw" in the error handling code.

I am not sure how to fix this bug. Any comment on removing the dereference of fw?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...

No, there is no bug there. It is a static checker false positive.

When you are reporting static checker warnings then please at least mention it is from static analsysis so we can know what we are dealing with.

Here is the code.

drivers/staging/greybus/bootrom.c 241 static int gb_bootrom_get_firmware(struct gb_operation *op) 242 { 243 struct gb_bootrom *bootrom = gb_connection_get_data(op->connection); 244 const struct firmware *fw; ^^^

245 struct gb_bootrom_get_firmware_request *firmware_request; 246 struct gb_bootrom_get_firmware_response *firmware_response; 247 struct device *dev = &op->connection->bundle->dev; 248 unsigned int offset, size; 249 enum next_request_type next_request; 250 int ret = 0; 251 252 /* Disable timeouts */ 253 gb_bootrom_cancel_timeout(bootrom); 254 255 if (op->request->payload_size != sizeof(*firmware_request)) { 256 dev_err(dev, "%s: Illegal size of get firmware request (%zu %zu)\n", 257 __func__, op->request->payload_size, 258 sizeof(*firmware_request)); 259 ret = -EINVAL; 260 goto queue_work;

"ret" is -EINVAL. "fw" is uninitialized.

261 } 262 263 mutex_lock(&bootrom->mutex); 264 265 fw = bootrom->fw; 266 if (!fw) { 267 dev_err(dev, "%s: firmware not available\n", __func__); 268 ret = -EINVAL; 269 goto unlock;

"ret" is -EINVAL. "fw" is NULL.

270 } 271

For the rest "fw" is valid.

272 firmware_request = op->request->payload; 273 offset = le32_to_cpu(firmware_request->offset); 274 size = le32_to_cpu(firmware_request->size); 275 276 if (offset >= fw->size || size > fw->size - offset) { 277 dev_warn(dev, "bad firmware request (offs = %u, size = %u)\n", 278 offset, size); 279 ret = -EINVAL; 280 goto unlock; 281 } 282 283 if (!gb_operation_response_alloc(op, sizeof(*firmware_response) + size, 284 GFP_KERNEL)) { 285 dev_err(dev, "%s: error allocating response\n", __func__); 286 ret = -ENOMEM; 287 goto unlock; 288 } 289 290 firmware_response = op->response->payload; 291 memcpy(firmware_response->data, fw->data + offset, size); 292 293 dev_dbg(dev, "responding with firmware (offs = %u, size = %u)\n", 294 offset, size); 295 296 unlock: 297 mutex_unlock(&bootrom->mutex); 298 299 queue_work: 300 /* Refresh timeout */ 301 if (!ret && (offset + size == fw->size)) ^^^^^ The "!ret" is only true when "fw" is valid.

302 next_request = NEXT_REQ_READY_TO_BOOT; 303 else 304 next_request = NEXT_REQ_GET_FIRMWARE; 305 306 gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS); 307 308 return ret; 309 }

regards, dan carpenter

On 21-06-22, 16:40, Johan Hovold wrote:

On Tue, Jun 21, 2022 at 10:36:04PM +0800, Dongliang Mu wrote:

...

Hi maintainers,

I would like to send one bug report.

In gb_bootrom_get_firmware, if the first branch is satisfied, it will go to queue_work, leading to the dereference of uninitialized const variable "fw". If the second branch is satisfied, it will go to unlock with fw as NULL pointer, leading to a NULL Pointer Dereference.

This sounds like the false positive that checkers keep tripping over.

Please double check your analysis and search the archives first.

And everytime I get this report, I wonder if I should change the code to be a bit simpler :)