From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.6607.1581931938806186611 for ; Mon, 17 Feb 2020 01:32:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=O9jz1YGl; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581931937; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vrYjBJ2R/hKLmwvfyEEItphN2PmT2oqHHOqWzVm0L8g=; b=O9jz1YGlGpKpyszr/Tf8JBElfjenQ0FMXjwsCdCN7TS8igFRqBWxM7oNDp/M7Xl6F0Ra/Y mXYD+7HB8c83fkG2h+RrJIjAe49orQggSkmR4PlaujpBEFtj9FvYs9bpFXwjHddQ2L+366 AkmNd078+q+EESbE+EbOqJkGmh79WZ8= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-98-NJfHNs3UMJGIVft8ZXI4Bg-1; Mon, 17 Feb 2020 04:32:16 -0500 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 02A018017CC; Mon, 17 Feb 2020 09:32:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-195.ams2.redhat.com [10.36.116.195]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0BE8710013A1; Mon, 17 Feb 2020 09:32:10 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation To: devel@edk2.groups.io, philmd@redhat.com Cc: Jian J Wang , Hao A Wu , Eric Dong References: <20200213182935.26663-1-philmd@redhat.com> <20200213182935.26663-2-philmd@redhat.com> From: "Laszlo Ersek" Message-ID: <115eb81f-59ca-398c-bac8-445e9a516849@redhat.com> Date: Mon, 17 Feb 2020 10:32:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-MC-Unique: NJfHNs3UMJGIVft8ZXI4Bg-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 02/13/20 19:33, Philippe Mathieu-Daud=C3=A9 wrote: > On 2/13/20 7:29 PM, Philippe Mathieu-Daude wrote: >> Math expressions written in terms of SafeIntLib function calls >> are easily readable, making review trivial. Convert the truncation >> checks added by commit 322ac05f8 to SafeIntLib calls. >> >> Cc: Jian J Wang >> Cc: Hao A Wu >> Cc: Eric Dong >> Suggested-by: Laszlo Ersek >> Signed-off-by: Philippe Mathieu-Daude >> --- >> =C2=A0 .../DxeS3BootScriptLib.inf=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= |=C2=A0=C2=A0 1 + >> =C2=A0 .../InternalBootScriptLib.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |= =C2=A0=C2=A0 1 + >> =C2=A0 .../PiDxeS3BootScriptLib/BootScriptSave.c=C2=A0=C2=A0=C2=A0=C2=A0= | 114 +++++++++++------- >> =C2=A0 3 files changed, 73 insertions(+), 43 deletions(-) >> >> diff --git >> a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> index 2b894c99da55..698039fe8e69 100644 >> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >> @@ -40,15 +40,16 @@ [Packages] >> =C2=A0 [LibraryClasses] >> =C2=A0=C2=A0=C2=A0 UefiBootServicesTableLib >> =C2=A0=C2=A0=C2=A0 BaseLib >> =C2=A0=C2=A0=C2=A0 BaseMemoryLib >> =C2=A0=C2=A0=C2=A0 TimerLib >> =C2=A0=C2=A0=C2=A0 DebugLib >> =C2=A0=C2=A0=C2=A0 PcdLib >> =C2=A0=C2=A0=C2=A0 UefiLib >> =C2=A0=C2=A0=C2=A0 SmbusLib >> =C2=A0=C2=A0=C2=A0 PciSegmentLib >> =C2=A0=C2=A0=C2=A0 IoLib >> =C2=A0=C2=A0=C2=A0 LockBoxLib >> +=C2=A0 SafeIntLib >> =C2=A0 =C2=A0 [Protocols] >> =C2=A0=C2=A0=C2=A0 gEfiSmmBase2ProtocolGuid=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0 ## SOMETIMES_CONSUMES >> diff --git >> a/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h >> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h >> index 9485994087d0..7513220c15ac 100644 >> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h >> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScriptLib.h >> @@ -1,49 +1,50 @@ >> =C2=A0 /** @file >> =C2=A0=C2=A0=C2=A0 Support for S3 boot script lib. This file defined som= e internal >> macro and internal >> =C2=A0=C2=A0=C2=A0 data structure >> =C2=A0 =C2=A0=C2=A0=C2=A0 Copyright (c) 2006 - 2016, Intel Corporation. = All rights >> reserved.
>> =C2=A0 =C2=A0=C2=A0=C2=A0 SPDX-License-Identifier: BSD-2-Clause-Patent >> =C2=A0 =C2=A0 **/ >> =C2=A0 #ifndef __INTERNAL_BOOT_SCRIPT_LIB__ >> =C2=A0 #define __INTERNAL_BOOT_SCRIPT_LIB__ >> =C2=A0 =C2=A0 #include >> =C2=A0 =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 =C2=A0 #include >> =C2=A0 =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> =C2=A0 #include >> +#include >> =C2=A0 =C2=A0 #include "BootScriptInternalFormat.h" >> =C2=A0 =C2=A0 #define MAX_IO_ADDRESS 0xFFFF >> =C2=A0 =C2=A0 // >> =C2=A0 // Macro to convert a UEFI PCI address + segment to a PCI Segment >> Library PCI address >> =C2=A0 // >> =C2=A0 #define PCI_ADDRESS_ENCODE(S, A) PCI_SEGMENT_LIB_ADDRESS( \ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 S, \ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((((U= INTN)(A)) & 0xff000000) >> >> 24), \ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((((U= INTN)(A)) & 0x00ff0000) >> >> 16), \ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((((U= INTN)(A)) & 0xff00) >> 8), \ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ((RSh= iftU64 ((A), 32) & 0xfff) | >> ((A) & 0xff)) \ >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ) >> diff --git >> a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> index 9315fc9f0188..d229263638fc 100644 >> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c >> @@ -995,55 +995,60 @@ EFIAPI >> =C2=A0 S3BootScriptSaveIoWrite ( >> =C2=A0=C2=A0=C2=A0 IN=C2=A0 S3_BOOT_SCRIPT_LIB_WIDTH=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Width, >> =C2=A0=C2=A0=C2=A0 IN=C2=A0 UINT64=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Address, >> =C2=A0=C2=A0=C2=A0 IN=C2=A0 UINTN=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Count, >> =C2=A0=C2=A0=C2=A0 IN=C2=A0 VOID=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *Buffer >> =C2=A0=C2=A0=C2=A0 ) >> =C2=A0 =C2=A0 { >> +=C2=A0 EFI_STATUS=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Status; >> =C2=A0=C2=A0=C2=A0 UINT8=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Le= ngth; >> =C2=A0=C2=A0=C2=A0 UINT8=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 *Script; >> =C2=A0=C2=A0=C2=A0 UINT8=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Wi= dthInByte; >> =C2=A0=C2=A0=C2=A0 EFI_BOOT_SCRIPT_IO_WRITE=C2=A0 ScriptIoWrite; >> =C2=A0 -=C2=A0 WidthInByte =3D (UINT8) (0x01 << (Width & 0x03)); >> +=C2=A0 Status =3D SafeUintnToUint8 (Count, &Length); >> +=C2=A0 if (EFI_ERROR (Status)) { >> +=C2=A0=C2=A0=C2=A0 return RETURN_OUT_OF_RESOURCES; >> +=C2=A0 } >> + >> +=C2=A0 Status =3D SafeUint8Mult (Length, 0x01 << (Width & 0x03), &Lengt= h); >> +=C2=A0 if (EFI_ERROR (Status)) { >> +=C2=A0=C2=A0=C2=A0 return RETURN_OUT_OF_RESOURCES; >> +=C2=A0 } >> =C2=A0 -=C2=A0 // >> -=C2=A0 // Truncation check >> -=C2=A0 // >> -=C2=A0 if ((Count > MAX_UINT8) || >> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 (WidthInByte * Count > MAX_UINT8 - sizeo= f >> (EFI_BOOT_SCRIPT_IO_WRITE))) { >> +=C2=A0 Status =3D SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_IO_WRIT= E), >> &Length); >> +=C2=A0 if (EFI_ERROR (Status)) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return RETURN_OUT_OF_RESOURCES; >> =C2=A0=C2=A0=C2=A0 } >> -=C2=A0 Length =3D (UINT8)(sizeof (EFI_BOOT_SCRIPT_IO_WRITE) + (WidthInB= yte * >> Count)); >> =C2=A0 =C2=A0=C2=A0=C2=A0 Script =3D S3BootScriptGetEntryAddAddress (Len= gth); >> =C2=A0=C2=A0=C2=A0 if (Script =3D=3D NULL) { >> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return RETURN_OUT_OF_RESOURCES; >> =C2=A0=C2=A0=C2=A0 } >> =C2=A0=C2=A0=C2=A0 // >> =C2=A0=C2=A0=C2=A0 // save script data >> =C2=A0=C2=A0=C2=A0 // >> =C2=A0=C2=A0=C2=A0 ScriptIoWrite.OpCode=C2=A0 =3D EFI_BOOT_SCRIPT_IO_WRI= TE_OPCODE; >> =C2=A0=C2=A0=C2=A0 ScriptIoWrite.Length=C2=A0 =3D Length; >> =C2=A0=C2=A0=C2=A0 ScriptIoWrite.Width=C2=A0=C2=A0 =3D Width; >> =C2=A0=C2=A0=C2=A0 ScriptIoWrite.Address =3D Address; >> =C2=A0=C2=A0=C2=A0 ScriptIoWrite.Count=C2=A0=C2=A0 =3D (UINT32) Count; >> =C2=A0=C2=A0=C2=A0 CopyMem ((VOID*)Script, (VOID*)&ScriptIoWrite, >> sizeof(EFI_BOOT_SCRIPT_IO_WRITE)); >> =C2=A0=C2=A0=C2=A0 CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_IO_= WRITE)), >> Buffer, WidthInByte * Count); >> =C2=A0 =C2=A0=C2=A0=C2=A0 SyncBootScript (Script); >> =C2=A0 =C2=A0=C2=A0=C2=A0 return RETURN_SUCCESS; >> =C2=A0 } >=20 > Oops wrong version (WidthInByte is uninitialized). Also -- if the MdeModulePkg maintainers like the appraoch in general --, you could eliminate the "WidthInByte * Count" multiplication from the CopyMem() call altogether. Instead, you could save the SafeUint8Mult() result in an intermediate variable, and reuse it here. Just an idea (again, only for the case when the MdeModulePkg actually like this refactoring). Thanks! Laszlo