RHEL-183405: [viostor] Validate blk_size to prevent divide-by-zero - #1586
RHEL-183405: [viostor] Validate blk_size to prevent divide-by-zero#1586harshapa-rh wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a validation check for the block size (blk_size) retrieved from the device configuration in viostor/virtio_stor_hw_helper.c to ensure it is not smaller than SECTOR_SIZE. If it is, a default value is used. The review feedback suggests correcting the format specifier in the debug print from %d to %u to match the unsigned type of the variable, and adjusting the brace placement to maintain consistent coding style.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Hi @harshapa-rh Please squash all the commits It is better to run clang-format locally first |
YanVugenfirer
left a comment
YanVugenfirer
left a comment
There was a problem hiding this comment.
Changes requested in the previous comment
|
BTW: in PR and commits use the parent Jira key: https://redhat.atlassian.net/browse/RHEL-183405 |
8a11510 to
fcebcbd
Compare
harshapa-rh
changed the title
YanVugenfirer
left a comment
YanVugenfirer
left a comment
There was a problem hiding this comment.
- Fix the commit message. The squashed commit message should contain only one message
- We want to validate that v is not zero. If it is zero, propagate error to initialization functions so they will fail.
- We probably cannot do anything useful on reconfiguration notification.
| if (CHECKBIT(adaptExt->features, VIRTIO_BLK_F_BLK_SIZE)) | ||
| { | ||
| virtio_get_config(&adaptExt->vdev, FIELD_OFFSET(blk_config, blk_size), &v, sizeof(v)); | ||
| if (v < SECTOR_SIZE) |
There was a problem hiding this comment.
- Fix the commit message. The squashed commit message should contain only one message
- We want to validate that v is not zero. If it is zero, propagate error to initialization functions so they will fail.
- We probably cannot do anything useful on reconfiguration notification.
ac46fcd to
c92b214
Compare
harshapa-rh
changed the title
513de0c to
11e7ccc
Compare
Signed-off-by: Harshal Patil <harshapa@redhat.com>
| virtio_get_config(&adaptExt->vdev, FIELD_OFFSET(blk_config, blk_size), &v, sizeof(v)); | ||
| if (v == 0) | ||
| { | ||
| RhelDbgPrint(TRACE_LEVEL_ERROR, "Invalid blk_size %u from device config, using default\n", v); |
There was a problem hiding this comment.
The debug print is misleading. Please remove the ", using default" part.
The failure of the function will cause the device not to initialize.
BTW: Did you test the fix by installing the driver? Also it is worth to return 0 on purpose from the host (use virtio-blk as secondary device in such case)
There was a problem hiding this comment.
I fixed the debug print. I was able to install the driver but I couldn't simulate this specific test case.
11e7ccc to
dc4becc
Compare
|
run tests |
| if (v == 0) | ||
| { | ||
| RhelDbgPrint(TRACE_LEVEL_ERROR, "Invalid blk_size %u from device config\n", v); | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
This early return means you get a partially updated adaptExt. I think this is unexpected, you always want a valid new state or the unchanged old state.
It probably doesn't matter when called from VirtIoFindAdapter() or VirtIoHwReinitialize(), where failure kills the whole device anyway, but VirtIoConfigUpdated() ignores the error silently. The driver would then keep partially updated values, but never notify StorPort about the changes, potentially making assumptions inconsistent between the two layers (though of course, both are now inconsistent with the device anyway).
The other, probably better option is making sure that the failure path in VirtIoConfigUpdated() actually leads to a disabled device, too.
|
Please update the commit message with the motivation for the fix. Is this purely theoretical or is there a way to actually end up in this situation? Please also state there why the chosen solution (fail device initialisation on error, but silently ignore the error on runtime updates) is the correct/desirable one. |
|
@kevmw While reproducing I noticed that standard QEMU already prevents this. It will reject these parameters. So this can either be closed as is to reduce redundancy or we can keep this for good measure after I make any suggested fixes. @YanVugenfirer please review the discussion and decide the best course of action. Thank you. |
|
Hardening can still be a good idea, so I'm not against addressing it even though it can't be reproduced with QEMU. The commit message should just point this out for clarity. |
3 participants
…de-by-zero