* Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware
@ 2022-06-21 14:36 Dongliang Mu
2022-06-21 14:40 ` Johan Hovold
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dongliang Mu @ 2022-06-21 14:36 UTC (permalink / raw)
To: vireshk, Johan Hovold, elder, Greg KH
Cc: greybus-dev, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware
2022-06-21 14:36 Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware Dongliang Mu
@ 2022-06-21 14:40 ` Johan Hovold
2022-06-22 2:19 ` Viresh Kumar
2022-06-21 14:48 ` Greg KH
2022-06-21 14:55 ` Dan Carpenter
2 siblings, 1 reply; 6+ messages in thread
From: Johan Hovold @ 2022-06-21 14:40 UTC (permalink / raw)
To: Dongliang Mu
Cc: vireshk, elder, Greg KH, greybus-dev, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware
2022-06-21 14:36 Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware Dongliang Mu
2022-06-21 14:40 ` Johan Hovold
@ 2022-06-21 14:48 ` Greg KH
2022-06-21 14:55 ` Dan Carpenter
2 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2022-06-21 14:48 UTC (permalink / raw)
To: Dongliang Mu
Cc: vireshk, Johan Hovold, elder, greybus-dev, linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware
2022-06-21 14:36 Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware Dongliang Mu
2022-06-21 14:40 ` Johan Hovold
2022-06-21 14:48 ` Greg KH
@ 2022-06-21 14:55 ` Dan Carpenter
2022-06-21 23:21 ` Dongliang Mu
2 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2022-06-21 14:55 UTC (permalink / raw)
To: Dongliang Mu
Cc: vireshk, Johan Hovold, elder, Greg KH, greybus-dev,
linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware
2022-06-21 14:55 ` Dan Carpenter
@ 2022-06-21 23:21 ` Dongliang Mu
0 siblings, 0 replies; 6+ messages in thread
From: Dongliang Mu @ 2022-06-21 23:21 UTC (permalink / raw)
To: Dan Carpenter
Cc: vireshk, Johan Hovold, elder, Greg KH, greybus-dev,
linux-staging, linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware
2022-06-21 14:40 ` Johan Hovold
@ 2022-06-22 2:19 ` Viresh Kumar
0 siblings, 0 replies; 6+ messages in thread
From: Viresh Kumar @ 2022-06-22 2:19 UTC (permalink / raw)
To: Johan Hovold
Cc: Dongliang Mu, vireshk, elder, Greg KH, greybus-dev,
linux-staging, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-06-22 2:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-21 14:36 Unitialized Variable and Null Pointer Dereference bug in gb_bootrom_get_firmware Dongliang Mu
2022-06-21 14:40 ` Johan Hovold
2022-06-22 2:19 ` Viresh Kumar
2022-06-21 14:48 ` Greg KH
2022-06-21 14:55 ` Dan Carpenter
2022-06-21 23:21 ` Dongliang Mu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.