On 14/02/2023 13:01, Gerd Hoffmann wrote:
On Mon, Feb 13, 2023 at 04:15:30PM +0000, Michael Brown wrote:
On 13/02/2023 15:48, Michael Kubacki wrote:
@@ -1608,9 +1610,12 @@ ParseAndAddExistingSmbiosTable (
//
// Make sure not to access memory beyond SmbiosEnd
//
- if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||
- (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
- {
+ Status = SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE),
&SafeIntResult);
+ if (EFI_ERROR (Status)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if ((SafeIntResult > (UINTN)SmbiosEnd.Raw) || (SafeIntResult <
(UINTN)Smbios.Raw)) {
return EFI_INVALID_PARAMETER;
}
Could this not be expressed much more cleanly as just:
if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < sizeof (SMBIOS_STRUCTURE)) {
return EFI_INVALID_PARAMETER;
}
without the need to hide a basic arithmetic operation behind an ugly wrapper
function and drag in an external library?
Well, the advantage of using SafeIntLib is that (a) it is obvious even
to untrained code readers what is going on here, and (b) it is hard to
get it wrong.
When looking at the quality some of the patches being posted to the list
have I'd say that is important to consider even if you personally have
no problems avoiding integer overflows without the help of SafeIntLib.
Fair enough. But in that case it should be used in a way that makes it
clear what it's actually doing. Specifically, the check for
if (... || (SafeIntResult < (UINTN)Smbios.Raw)) {
...
}
then becomes meaningless, since the whole point of SafeUintnAdd() is
that it cannot result in this kind of unsigned integer wraparound. The
code as modified by this patch is *harder* to understand, because the
reader has to dig through to figure out why this check still exists,
look at the implementation of SafeUintnAdd() to see what is meant by
"safe" in this context, and finally come to the conclusion that the
remaining underflow check is redundant code that should have been removed.
The reader is also going to be confused by the fact that the code as
modified by this patch then contains two separate methods for checking
for integer overflows, since the patch does not similarly update the
very similar code found almost immediately afterwards:
if ((Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) >
SmbiosEnd.Raw) ||
(Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) <
Smbios.Raw))
{
return EFI_INVALID_PARAMETER;
}
Lastly: the actual operation being made safe here is "check that buffer
contains at least this much data remaining". This is most obviously
done by calculating the remaining buffer space (i.e.
(UINTN)(SmbiosEnd.Raw - Smbios.Raw)) and comparing against that. That
logic is clear, simple, and obviously correct at first glance. Using
the SafeUintnAdd() call does something that *is* mathematically
equivalent to this check, but where the logic is much harder to parse.
I'd prefer it if the code were updated to avoid SafeUintnAdd()
altogether. But if not, then at a minimum the redundant check should be
removed, and the calculation involving Smbios.Hdr->Length should also be
updated to use SafeUintnAdd().
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100177): https://edk2.groups.io/g/devel/message/100177
Mute This Topic: https://groups.io/mt/96938294/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
