From: Erich McMillan <emcmillan@microsoft.com>
Details for these CodeQL alerts can be found here:
- Pointer overflow check (cpp/pointer-overflow-check):
- https://cwe.mitre.org/data/definitions/758.html
- Potential buffer overflow check (cpp/potential-buffer-overflow):
- https://cwe.mitre.org/data/definitions/676.html
CodeQL alert:
- Line 1612 in MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
- Type: Pointer overflow check
- Severity: Low
- Problem: Range check relying on pointer overflow
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Erich McMillan <emcmillan@microsoft.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Co-authored-by: Michael Kubacki <michael.kubacki@microsoft.com>
Signed-off-by: Erich McMillan <emcmillan@microsoft.com>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
---
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c | 11 ++++++++---
MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 1 +
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
index 1d43adc7662c..c1da2adc296b 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#include "SmbiosDxe.h"
+#include <Library/SafeIntLib.h>
//
// Module Global:
@@ -1594,6 +1595,7 @@ ParseAndAddExistingSmbiosTable (
CHAR8 *String;
EFI_SMBIOS_HANDLE SmbiosHandle;
SMBIOS_STRUCTURE_POINTER SmbiosEnd;
+ UINTN SafeIntResult;
mPrivateData.Smbios.MajorVersion = MajorVersion;
mPrivateData.Smbios.MinorVersion = MinorVersion;
@@ -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;
}
diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
index c03915a6921f..8b7c74694775 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
@@ -42,6 +42,7 @@ [LibraryClasses]
DebugLib
PcdLib
HobLib
+ SafeIntLib
[Protocols]
gEfiSmbiosProtocolGuid ## PRODUCES
--
2.28.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100102): https://edk2.groups.io/g/devel/message/100102
Mute This Topic: https://groups.io/mt/96938294/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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?
The almost-identical check performed in the code immediately below (that
takes Smbios.Hdr->Length into account) should also be updated to e.g.:
if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < (Smbios.Hdr->Length + 2)) {
...
}
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100116): https://edk2.groups.io/g/devel/message/100116
Mute This Topic: https://groups.io/mt/96938294/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100174): https://edk2.groups.io/g/devel/message/100174
Mute This Topic: https://groups.io/mt/96938294/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Either approach works for me.
I understand the desire to avoid code bloat that comes with the library.
The most common classes of issues I see at the moment are asserts being
misused for error handling (which is significant), general issues with
integer conversion/evaluation, and unsafe arithmetic operations.
I suppose this is in the spirit of Gerd's comment as I thought
additional library usage might increase awareness to help with the latter.
Do the MdeModulePkg / SMBIOS maintainers have a preference?
Thanks,
Michael
On 2/14/2023 8:01 AM, 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.
>
> take care,
> Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#100178): https://edk2.groups.io/g/devel/message/100178
Mute This Topic: https://groups.io/mt/96938294/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi, > [ ... details snipped ... ] > > 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(). Fully agreeing to that. take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100180): https://edk2.groups.io/g/devel/message/100180 Mute This Topic: https://groups.io/mt/96938294/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
I know the second case was missed, that will be updated. I agree calculating the remaining buffer space is more straightforward here without the library so I'll go with that approach in a v4 of the series. Thanks for the detailed feedback. On 2/14/2023 9:11 AM, Gerd Hoffmann wrote: > Hi, > >> [ ... details snipped ... ] >> >> 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(). > > Fully agreeing to that. > > take care, > Gerd > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#100182): https://edk2.groups.io/g/devel/message/100182 Mute This Topic: https://groups.io/mt/96938294/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.