[vioscsi] Limit maximum LUN index in TransportReset() and ParamChange()#1512
[vioscsi] Limit maximum LUN index in TransportReset() and ParamChange()#1512benyamin-codez wants to merge 1 commit intovirtio-win:masterfrom
Conversation
Summary of ChangesHello @benyamin-codez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the vioscsi driver by addressing a potential issue where LUN indices exceeding the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
[Jenkins CI]: Can one of the admins verify this patch? |
![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 correctly addresses a potential integer truncation vulnerability by adding bounds checking for the LUN index in TransportReset() and ParamChange(). This is a good security enhancement. I've provided a couple of suggestions to improve code correctness and readability, and another to address code duplication for better maintainability.
vioscsi/vioscsi.c
Outdated
| if (lun_candidate > SCSI_MAXIMUM_LUNS_PER_TARGET - 1) | ||
| { | ||
| RhelDbgPrint(TRACE_LEVEL_WARNING, | ||
| " The LUN specified (%d) is out-of-bounds." | ||
| " Storport can only address a maximum of %d LUNs." | ||
| " The event reason was 0x%x." | ||
| " Returning...\n", | ||
| lun_candidate, | ||
| SCSI_MAXIMUM_LUNS_PER_TARGET, | ||
| evt->reason); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This bounds check is a good security improvement. For correctness and readability, I suggest two minor changes:
- Use the
%uformat specifier forlun_candidateinRhelDbgPrintsince it's an unsigned type (USHORT). - Change the comparison to
lun_candidate >= SCSI_MAXIMUM_LUNS_PER_TARGET, which is equivalent and arguably more readable than> SCSI_MAXIMUM_LUNS_PER_TARGET - 1.
Additionally, this validation logic is duplicated from TransportReset. To improve maintainability, consider extracting this logic into a static helper function.
if (lun_candidate >= SCSI_MAXIMUM_LUNS_PER_TARGET)
{
RhelDbgPrint(TRACE_LEVEL_WARNING,
" The LUN specified (%u) is out-of-bounds."
" Storport can only address a maximum of %d LUNs."
" The event reason was 0x%x."
" Returning...\n",
lun_candidate,
SCSI_MAXIMUM_LUNS_PER_TARGET,
evt->reason);
return;
}There was a problem hiding this comment.
Will do 1 & 2...
Re static helper - this would only be for two callers. Happy to be guided on this by maintainers...
The 'Lun' variable in TransportReset() and ParamChange() is rightly declared
as a UCHAR value. However, the value is derived from two UCHAR members of a
'VirtIOSCSIEvent' struct ('lun[2]' and 'lun[3]'). When the 'lun[2]' member is
left shifted 8 bits it is promoted to type USHORT, and then bitwise OR'ed with
'lun[3]'. This leads to an integer truncation if the LUN index is greater than
256. Whilst Storport does not support more than 255 LUNs per Target as defined
by SCSI_MAXIMUM_LUNS_PER_TARGET, a malicious or misconfigured hypervisor could
send an event for a higher LUN index. In TransportReset() this could result in
another LUN being reset, and in ParamChange() this could result in processing
parameter changes for the wrong LUN.
The solution in this commit is to first obtain the LUN index via new USHORT
variable 'lun_candidate' and then check if it is within the bounds of
SCSI_MAXIMUM_LUNS_PER_TARGET. If out-of-bounds, we log a warning message -
including the reason for the event - and return without further processing.
Otherwise we cast 'lun_candidate` to 'Lun' as a UCHAR and continue.
Signed-off-by: benyamin-codez <115509179+benyamin-codez@users.noreply.github.com>
benyamin-codez
force-pushed
the
vioscsi-max-lun-oob-protect
branch
from
72a4c50 to
0411ea3
Compare
|
I've only tested this for normal operation. To trigger the fault path this will likely require testing with a maliciously crafted version of say cc: @kevmw |
|
@benyamin-codez I think maybe I am missing something - Lun is calculated, but never used. So I think this PR is redundant. |
Wow..! That got past me... I don't think you're missing anything - I just followed AI up the garden path... 8^d Not just On the SCSI side, iinm, we use a vendor-specific addressing method derived from SAM-4, per the VirtIO spec:
Most addressing methods that make use of the additional bytes typically left shift each byte to derive a concatenated value, so that behaviour is not surprising. Having said all that, Storport is still limited to 255 LUNs (0 to 254)... I do see some value to adding some tracing here to make use of these variables. It could help identify various problems in these pathways. For example, ejecting the hard disk currently doesn't work. At a guess, that process seems like it might at least touch one of these functions - but maybe not. A log entry for However, if we don't use Please do let me know how you would like to proceed. Best regards, |
|
Hi @benyamin-codez. Let's remove Lun. Thanks! |
2 participants
The
Lunvariable inTransportReset()andParamChange()is rightly declared as a UCHAR value. However, the value is derived from two UCHAR members of aVirtIOSCSIEventstruct (lun[2]andlun[3]). When thelun[2]member is left shifted 8 bits it is promoted to type USHORT, and then bitwise OR'ed withlun[3]. This leads to an integer truncation if the LUN index is greater than 256. Whilst Storport does not support more than 255 LUNs per Target as defined bySCSI_MAXIMUM_LUNS_PER_TARGET, a malicious or misconfigured hypervisor could send an event for a higher LUN index. InTransportReset()this could result in another LUN being reset, and inParamChange()this could result in processing parameter changes for the wrong LUN.The solution in this commit is to first obtain the LUN index via new USHORT variable
lun_candidateand then check if it is within the bounds ofSCSI_MAXIMUM_LUNS_PER_TARGET. If out-of-bounds, we log a warning message - including the reason for the event - and return without further processing. Otherwise we castlun_candidatetoLunas a UCHAR and continue.