Details
-
Bug
-
Resolution: Fixed
-
Major
-
55,173
Description
r73488 - boot/freeldr/freeldr/arch/i386/hardware.c (appears same thru at least r73501, latest at this writing)
(The following is from code observation/review, perhaps I am mistaken.)
This concerns the routine DetectPnpBios() and routines it calls.
A size for allocation is computed,
Size = sizeof(CM_PARTIAL_RESOURCE_LIST)
+ sizeof(CM_PNP_BIOS_INSTALLATION_CHECK) + (NodeSize * NodeCount);
It later has a tracking size variable PnpBufferSize, which it uses to track how much of a buffer has been used, with the expression
if (PnpBufferSize + DeviceNode->Size > Size)
but, 'Size' on the right of that expression appears to include overhead sizeof(CM_PARTIAL_RESOURCE_LIST), which the buffer walking pointer 'Ptr' has already skipped over ('Ptr = (char *)(PartialResourceList + 1);').
Hence, it appears that the tracking variable Ptr could be moved past the end of the actually available area by as much as sizeof(CM_PARTIAL_RESOURCE_LIST).
This likely would only ever cause a real problem if a bios did return drive information for more drives than were allowed for in the earlier calls that resulted in the expression 'NodeSize * NodeCount', or, if the cumulative 'DeviceNode->Size's is ever greater than the total computed 'NodeSize*NodeCount', which would mean DeviceNode->Size was not always equal to the 'NodeSize' value apparently returned from the call PnpBiosGetDeviceNodeCount(&NodeSize, &NodeCount);
If it is possible that NodeSize != DeviceNode->Size, then probably a loop should be performed prior to the allocation to determine the actual sizes involved, and use that to allocate the buffer.
The current code assumes that NodeSize*NodeCount will always be greater than or equal to the accumulation of all actual 'DeviceNode->Size's.
(Don't appear to be any active 'assert()'s in the boot code, but some sort of check that DeviceNode->Size == NodeSize might be comforting, and error/remediation taken if not so. If this is deemed not useful, then I would also question why the check mentioned above
'if (PnpBufferSize + DeviceNode->Size > Size)'
is present, as it should also not be necessary if DeviceNode->Size is always less than or equal to NodeSize obtained from 'PnpBiosGetDeviceNodeCount(&NodeSize, &NodeCount);' and used in buffer size for allocation.)
Attachments
Issue Links
- blocks
-
CORE-12802 Review checks for failure of FrLdrHeapAllocateEx()/FrLdrHeapAlloc()/FrLdrTempAlloc() (and wrappers) calls
- Open
- relates to
-
CORE-10146 Incorrect handling of CM_RESOURCE_LIST
- Resolved