From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) by mx.groups.io with SMTP id smtpd.web11.927.1582009843455731283 for ; Mon, 17 Feb 2020 23:10:44 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20161025 header.b=uIMp7l7b; spf=pass (domain: gmail.com, ip: 209.85.167.66, mailfrom: newexplorerj@gmail.com) Received: by mail-lf1-f66.google.com with SMTP id b15so13699390lfc.4 for ; Mon, 17 Feb 2020 23:10:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2LaD3hWaF8liZqte+BDbiU9r6FuxlydoMWqy7vfQ4zQ=; b=uIMp7l7bNQ4n2FBv7tdHbn4k01J/QsKjIWdayctsOCZ/sYaHWHJOI0dSmiGTMvvi+W BFYLq8u9vXpG5FCZJ9288wQ92t5KpX4i8CLMor2kkWIS7orrogxPb/AoN3FQ5SwkT79K 6K5yYUDY0jv872xmaD7iw0ZwCN42tzlWbkkpxf+SiustQ7EosZHYMpmPQz3zICNjlzs9 tkCARoKLrKajiKU+vLhnOhDVSqmDO9nxysKvEeFQ/5aWAxiW7XWy+ZkcyD2zEzOjZYGY GGEpBCZ68IqgSGIh0F1O0gccVTa9L04WSKMOy9sjQpdyY+Pt3ul02mzP+Hg1kAaL20ii 9fxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2LaD3hWaF8liZqte+BDbiU9r6FuxlydoMWqy7vfQ4zQ=; b=K6qqWic/l0YceRwhH2ID9wD73MJMgT2Ea0flBWmJ2ONjyHFWV6D8aFUZoNgNesaJQc YycOfK5z4gqyT9B3bTk5wvshtPKHBIJehSnXE48Wo1kdk0/vjJUimMUoOqtshMLyjF8v /EFcSjPZmRCOZNRLVUAb28FXOMCRihX+5DVqY8KgqgZNtT0dhC5gvn64iNtH4STe2R/r 61DHa+3dtj47D3Fdcle5QSqhrWstLos16j7PK3t2EPa7HrLenSxIt+3MovUR7+B/c2OD rdKXoE0rXE9cIchSIQExdd1Y724KEKUhSmNnfrnMXkKE3BaSmMQHWVBj8XZoWcyZ8o8R Xd5A== X-Gm-Message-State: APjAAAUf16I/lS4oN1gLScB+ax+9LbBxykkroCcSbn8DYd7QHALM2Pu4 wxHFvl4exDU9ttgoTq0hedJvENmj29gSeNSn08qxkS+YY+o= X-Google-Smtp-Source: APXvYqyzdVqvs/nHZO+Y9XzVoUi9QyYU4h5Wu+PaKCH3jYZUADF1SItoCP/tSeSlp16gHtFrl27c+CeSJge/1HO+QWk= X-Received: by 2002:a19:dc1e:: with SMTP id t30mr9702989lfg.34.1582009841278; Mon, 17 Feb 2020 23:10:41 -0800 (PST) MIME-Version: 1.0 References: <20200213182935.26663-1-philmd@redhat.com> <20200213182935.26663-2-philmd@redhat.com> <115eb81f-59ca-398c-bac8-445e9a516849@redhat.com> In-Reply-To: <115eb81f-59ca-398c-bac8-445e9a516849@redhat.com> From: "GuoMinJ" Date: Tue, 18 Feb 2020 15:10:29 +0800 Message-ID: Subject: Re: [edk2-devel] [RFC PATCH 1/1] MdeModulePkg/PiDxeS3BootScriptLib: Use SafeIntLib to avoid truncation To: devel@edk2.groups.io, lersek@redhat.com Cc: philmd@redhat.com, Jian J Wang , Hao A Wu , Eric Dong Content-Type: multipart/alternative; boundary="0000000000009ee40b059ed45efb" --0000000000009ee40b059ed45efb Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I agree with this comment. Laszlo Ersek =E4=BA=8E2020=E5=B9=B42=E6=9C=8817=E6=97= =A5=E5=91=A8=E4=B8=80 =E4=B8=8B=E5=8D=885:32=E5=86=99=E9=81=93=EF=BC=9A > 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 > >> --- > >> .../DxeS3BootScriptLib.inf | 1 + > >> .../InternalBootScriptLib.h | 1 + > >> .../PiDxeS3BootScriptLib/BootScriptSave.c | 114 +++++++++++----= --- > >> 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.in= f > >> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.in= f > >> @@ -40,15 +40,16 @@ [Packages] > >> [LibraryClasses] > >> UefiBootServicesTableLib > >> BaseLib > >> BaseMemoryLib > >> TimerLib > >> DebugLib > >> PcdLib > >> UefiLib > >> SmbusLib > >> PciSegmentLib > >> IoLib > >> LockBoxLib > >> + SafeIntLib > >> [Protocols] > >> gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUM= ES > >> 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 @@ > >> /** @file > >> Support for S3 boot script lib. This file defined some internal > >> macro and internal > >> data structure > >> Copyright (c) 2006 - 2016, Intel Corporation. All rights > >> reserved.
> >> SPDX-License-Identifier: BSD-2-Clause-Patent > >> **/ > >> #ifndef __INTERNAL_BOOT_SCRIPT_LIB__ > >> #define __INTERNAL_BOOT_SCRIPT_LIB__ > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> #include > >> +#include > >> #include "BootScriptInternalFormat.h" > >> #define MAX_IO_ADDRESS 0xFFFF > >> // > >> // Macro to convert a UEFI PCI address + segment to a PCI Segment > >> Library PCI address > >> // > >> #define PCI_ADDRESS_ENCODE(S, A) PCI_SEGMENT_LIB_ADDRESS( \ > >> S, \ > >> ((((UINTN)(A)) & 0xff000000) >> > >> 24), \ > >> ((((UINTN)(A)) & 0x00ff0000) >> > >> 16), \ > >> ((((UINTN)(A)) & 0xff00) >> 8), = \ > >> ((RShiftU64 ((A), 32) & 0xfff) | > >> ((A) & 0xff)) \ > >> ) > >> 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 > >> S3BootScriptSaveIoWrite ( > >> IN S3_BOOT_SCRIPT_LIB_WIDTH Width, > >> IN UINT64 Address, > >> IN UINTN Count, > >> IN VOID *Buffer > >> ) > >> { > >> + EFI_STATUS Status; > >> UINT8 Length; > >> UINT8 *Script; > >> UINT8 WidthInByte; > >> EFI_BOOT_SCRIPT_IO_WRITE ScriptIoWrite; > >> - WidthInByte =3D (UINT8) (0x01 << (Width & 0x03)); > >> + Status =3D SafeUintnToUint8 (Count, &Length); > >> + if (EFI_ERROR (Status)) { > >> + return RETURN_OUT_OF_RESOURCES; > >> + } > >> + > >> + Status =3D SafeUint8Mult (Length, 0x01 << (Width & 0x03), &Length)= ; > >> + if (EFI_ERROR (Status)) { > >> + return RETURN_OUT_OF_RESOURCES; > >> + } > >> - // > >> - // Truncation check > >> - // > >> - if ((Count > MAX_UINT8) || > >> - (WidthInByte * Count > MAX_UINT8 - sizeof > >> (EFI_BOOT_SCRIPT_IO_WRITE))) { > >> + Status =3D SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_IO_WRITE)= , > >> &Length); > >> + if (EFI_ERROR (Status)) { > >> return RETURN_OUT_OF_RESOURCES; > >> } > >> - Length =3D (UINT8)(sizeof (EFI_BOOT_SCRIPT_IO_WRITE) + (WidthInByt= e * > >> Count)); > >> Script =3D S3BootScriptGetEntryAddAddress (Length); > >> if (Script =3D=3D NULL) { > >> return RETURN_OUT_OF_RESOURCES; > >> } > >> // > >> // save script data > >> // > >> ScriptIoWrite.OpCode =3D EFI_BOOT_SCRIPT_IO_WRITE_OPCODE; > >> ScriptIoWrite.Length =3D Length; > >> ScriptIoWrite.Width =3D Width; > >> ScriptIoWrite.Address =3D Address; > >> ScriptIoWrite.Count =3D (UINT32) Count; > >> CopyMem ((VOID*)Script, (VOID*)&ScriptIoWrite, > >> sizeof(EFI_BOOT_SCRIPT_IO_WRITE)); > >> CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_IO_WRITE)), > >> Buffer, WidthInByte * Count); > >> SyncBootScript (Script); > >> return RETURN_SUCCESS; > >> } > > > > 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 > > >=20 > > --0000000000009ee40b059ed45efb Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I agree with this comment.

Laszlo Ersek &l= t;lersek@redhat.com> =E4=BA=8E2= 020=E5=B9=B42=E6=9C=8817=E6=97=A5=E5=91=A8=E4=B8=80 =E4=B8=8B=E5=8D=885:32= =E5=86=99=E9=81=93=EF=BC=9A
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 truncatio= n
>> checks added by commit 322ac05f8 to SafeIntLib calls.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Suggested-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daude <philmd@redhat.com>
>> ---
>> =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.in= f
>> b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.in= f
>> index 2b894c99da55..698039fe8e69 100644
>> --- a/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLi= b.inf
>> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLi= b.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/InternalBootScrip= tLib.h
>> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/InternalBootScrip= tLib.h
>> @@ -1,49 +1,50 @@
>> =C2=A0 /** @file
>> =C2=A0=C2=A0=C2=A0 Support for S3 boot script lib. This file defi= ned some 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 Corpor= ation. All rights
>> reserved.<BR>
>> =C2=A0 =C2=A0=C2=A0=C2=A0 SPDX-License-Identifier: BSD-2-Clause-P= atent
>> =C2=A0 =C2=A0 **/
>> =C2=A0 #ifndef __INTERNAL_BOOT_SCRIPT_LIB__
>> =C2=A0 #define __INTERNAL_BOOT_SCRIPT_LIB__
>> =C2=A0 =C2=A0 #include <PiDxe.h>
>> =C2=A0 =C2=A0 #include <Guid/EventGroup.h>
>> =C2=A0 #include <Protocol/SmmBase2.h>
>> =C2=A0 #include <Protocol/DxeSmmReadyToLock.h>
>> =C2=A0 #include <Protocol/SmmReadyToLock.h>
>> =C2=A0 #include <Protocol/SmmExitBootServices.h>
>> =C2=A0 #include <Protocol/SmmLegacyBoot.h>
>> =C2=A0 =C2=A0 #include <Library/S3BootScriptLib.h>
>> =C2=A0 =C2=A0 #include <Library/UefiBootServicesTableLib.h>=
>> =C2=A0 #include <Library/BaseLib.h>
>> =C2=A0 #include <Library/PcdLib.h>
>> =C2=A0 #include <Library/SmbusLib.h>
>> =C2=A0 #include <Library/IoLib.h>
>> =C2=A0 #include <Library/PciSegmentLib.h>
>> =C2=A0 #include <Library/DebugLib.h>
>> =C2=A0 #include <Library/BaseMemoryLib.h>
>> =C2=A0 #include <Library/TimerLib.h>
>> =C2=A0 #include <Library/UefiLib.h>
>> =C2=A0 #include <Library/LockBoxLib.h>
>> +#include <Library/SafeIntLib.h>
>> =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 ((((UINTN)(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 ((((UINTN)(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 ((((UINTN)(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 ((RShiftU64 ((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<= br> >> +++ b/MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c<= br> >> @@ -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,<= br> >> =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 *Buf= fer
>> =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 Length;
>> =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 WidthInByte;
>> =C2=A0=C2=A0=C2=A0 EFI_BOOT_SCRIPT_IO_WRITE=C2=A0 ScriptIoWrite;<= br> >> =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 &a= mp; 0x03), &Length);
>> +=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_UIN= T8 - sizeof
>> (EFI_BOOT_SCRIPT_IO_WRITE))) {
>> +=C2=A0 Status =3D SafeUint8Add (Length, sizeof (EFI_BOOT_SCRIPT_= IO_WRITE),
>> &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) + (W= idthInByte *
>> Count));
>> =C2=A0 =C2=A0=C2=A0=C2=A0 Script =3D S3BootScriptGetEntryAddAddre= ss (Length);
>> =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_WRITE_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) C= ount;
>> =C2=A0=C2=A0=C2=A0 CopyMem ((VOID*)Script, (VOID*)&ScriptIoWr= ite,
>> sizeof(EFI_BOOT_SCRIPT_IO_WRITE));
>> =C2=A0=C2=A0=C2=A0 CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCR= IPT_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 }
>
> Oops wrong version (WidthInByte is uninitialized).

Also -- if the MdeModulePkg maintainers like the appraoch in general --, you could eliminate the "WidthInByte * Count" multiplication fro= m 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=C2=A0 MdeModulePkg actuall= y
like this refactoring).

Thanks!
Laszlo




--0000000000009ee40b059ed45efb--