linux-kernel - Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220621145523.GJ16517@kadam>
Date:   Tue, 21 Jun 2022 17:55:23 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Dongliang Mu <mudongliangabcd@...il.com>
Cc:     vireshk@...nel.org, Johan Hovold <johan@...nel.org>,
        elder@...nel.org, Greg KH <gregkh@...uxfoundation.org>,
        greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: Unitialized Variable and Null Pointer Dereference bug in
 gb_bootrom_get_firmware

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