Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

[Dan Carpenter <dan.carpenter@oracle.com>]

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/?id=a4293e1d4e6416477976ee3bd248589d3fc4bb19

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