Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

[Dongliang Mu]

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

--
My best regards to you.

     No System Is Safe!
     Dongliang Mu

Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

[Johan Hovold]

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

Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

[Viresh Kumar]

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 :)

-- 
viresh

Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

[Greg KH]

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?

As Johan said, please fix up your tool that found this, it is not
working properly.

thanks,

greg k-h

Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

[Dan Carpenter]

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

Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware

[Dongliang Mu]

On Tue, Jun 21, 2022 at 10:55 PM Dan Carpenter <dan.carpenter@oracle.com> 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.
> >
> > 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.

Thanks Dan. I will do it next time.

I am just playing fun with the built-in coccinelle script. Static
analysis has many false positives. Sorry for my false alarm.

Thanks for your valuable feedback.

>
> 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
>