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
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/?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
On Tue, Jun 21, 2022 at 10:55 PM Dan Carpenter <[email protected]> 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
>
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