Hello Oded Gabbay,
The patch ac0ae6a96aa5: "habanalabs: add gaudi asic-dependent code"
from May 11, 2020, leads to the following Smatch static checker
warning:
drivers/misc/habanalabs/gaudi/gaudi.c:5568 gaudi_parse_cb_mmu()
error: 'parser->user_cb_size' from user is not capped properly
drivers/misc/habanalabs/gaudi/gaudi.c
5526 static int gaudi_parse_cb_mmu(struct hl_device *hdev,
5527 struct hl_cs_parser *parser)
5528 {
5529 u64 handle;
5530 u32 patched_cb_size;
5531 struct hl_cb *user_cb;
5532 int rc;
5533
5534 /*
5535 * The new CB should have space at the end for two MSG_PROT pkt:
5536 * 1. A packet that will act as a completion packet
5537 * 2. A packet that will generate MSI interrupt
5538 */
5539 if (parser->completion)
5540 parser->patched_cb_size = parser->user_cb_size +
5541 sizeof(struct packet_msg_prot) * 2;
5542 else
5543 parser->patched_cb_size = parser->user_cb_size;
5544
5545 rc = hl_cb_create(hdev, &hdev->kernel_mem_mgr, hdev->kernel_ctx,
5546 parser->patched_cb_size, false, false,
5547 &handle);
5548
5549 if (rc) {
5550 dev_err(hdev->dev,
5551 "Failed to allocate patched CB for DMA CS %d\n",
5552 rc);
5553 return rc;
5554 }
5555
5556 parser->patched_cb = hl_cb_get(&hdev->kernel_mem_mgr, handle);
5557 /* hl_cb_get should never fail */
5558 if (!parser->patched_cb) {
5559 dev_crit(hdev->dev, "DMA CB handle invalid 0x%llx\n", handle);
5560 rc = -EFAULT;
5561 goto out;
5562 }
5563
5564 /*
5565 * The check that parser->user_cb_size <= parser->user_cb->size was done
5566 * in validate_queue_index().
We are looking at cs_ioctl_default().
This comment is wrong. There is no check for this in validate_queue_index().
There is a check in get_cb_from_cs_chunk() but that function is only
called when is_kernel_allocated_cb is true.
I feel like we should check if "chunk->cb_size > cb->size" on all user
input. I think it is required. But even if it's not, it would make the
code easier for Smatch to understand.
5567 */
--> 5568 memcpy(parser->patched_cb->kernel_address,
5569 parser->user_cb->kernel_address,
5570 parser->user_cb_size);
^^^^^^^^^^^^^^^^^^^^
Otherwise *boom* user controlled buffer overflow.
5571
5572 patched_cb_size = parser->patched_cb_size;
5573
5574 /* Validate patched CB instead of user CB */
5575 user_cb = parser->user_cb;
5576 parser->user_cb = parser->patched_cb;
5577 rc = gaudi_validate_cb(hdev, parser, true);
5578 parser->user_cb = user_cb;
5579
5580 if (rc) {
5581 hl_cb_put(parser->patched_cb);
5582 goto out;
5583 }
5584
5585 if (patched_cb_size != parser->patched_cb_size) {
5586 dev_err(hdev->dev, "user CB size mismatch\n");
5587 hl_cb_put(parser->patched_cb);
5588 rc = -EINVAL;
5589 goto out;
5590 }
5591
5592 out:
5593 /*
5594 * Always call cb destroy here because we still have 1 reference
5595 * to it by calling cb_get earlier. After the job will be completed,
5596 * cb_put will release it, but here we want to remove it from the
5597 * idr
5598 */
5599 hl_cb_destroy(&hdev->kernel_mem_mgr, handle);
5600
5601 return rc;
5602 }
regards,
dan carpenter
