[edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts

[Michael Kubacki]

Thanks Mike. Erich, I'll include the update for this in the v2 series.

On 11/23/2022 8:28 PM, Michael D Kinney wrote:
> Hi Erich,
> 
> One comment below.
> 
> Mike
> 
>> -----Original Message-----
>> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki
>> Sent: Wednesday, November 9, 2022 9:33 AM
>> To: devel at edk2.groups.io
>> Cc: Bi, Dandan <dandan.bi at intel.com>; Erich McMillan <emcmillan at microsoft.com>; Wang, Jian J <jian.j.wang at intel.com>; Gao,
>> Liming <gaoliming at byosoft.com.cn>; Michael Kubacki <mikuback at linux.microsoft.com>; Zeng, Star <star.zeng at intel.com>; Gao,
>> Zhichao <zhichao.gao at intel.com>; Liu, Zhiguang <zhiguang.liu at intel.com>; Kubacki, Michael <michael.kubacki at microsoft.com>
>> Subject: [edk2-devel] [PATCH v1 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
>>
>> From: Erich McMillan <emcmillan at 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 at intel.com>
>> Cc: Erich McMillan <emcmillan at microsoft.com>
>> Cc: Jian J Wang <jian.j.wang at intel.com>
>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
>> Cc: Michael Kubacki <mikuback at linux.microsoft.com>
>> Cc: Star Zeng <star.zeng at intel.com>
>> Cc: Zhichao Gao <zhichao.gao at intel.com>
>> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
>> Co-authored-by: Michael Kubacki <michael.kubacki at microsoft.com>
>> Signed-off-by: Erich McMillan <emcmillan at microsoft.com>
>> ---
>>   MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 4 +++-
>>   MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 1 +
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
>> index 1d43adc7662c..03eca4e6b103 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;
>> @@ -1609,7 +1611,7 @@ ParseAndAddExistingSmbiosTable (
>>       // Make sure not to access memory beyond SmbiosEnd
>>       //
>>       if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) ||
> 
> The line above does the same unsafe add operation.
> The use of SafeUintnAdd()should be moved before the if statement
> and SafeIntResult should be used twice in the if statement.
> The check for EFI_ERROR from SafeUintnAdd() can be performed
> before the if statement.
> 
>> -        (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw))
>> +        EFI_ERROR (SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE), &SafeIntResult)))
>>       {
>>         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 (#96147): https://edk2.groups.io/g/devel/message/96147
>> Mute This Topic: https://groups.io/mt/94918085/1643496
>> Group Owner: devel+owner at edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [michael.d.kinney at intel.com]
>> -=-=-=-=-=-=
>>
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#96596): https://edk2.groups.io/g/devel/message/96596
Mute This Topic: https://groups.io/mt/94918085/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-