thanks laszlo I like your idea to eliminate the code duplication and fix all the problem by adding a new api in uefi lib.
If there is urgency to fix this specific issue, I have no problem on the enhancement. Reviewed by : jiewen....@intel.com thank you! Yao, Jiewen > 在 2018年7月12日，上午5:06，Laszlo Ersek <ler...@redhat.com> 写道： > >> On 07/11/18 18:06, Laszlo Ersek wrote: >>> On 07/11/18 17:44, Roman Bacik wrote: >>> Hi Laszlo, >>> >>> Thank you very much for your review and help. I would prefer the option 2b. >> >> Great, thanks! Let's wait for the SecurityPkg maintainers then, to give >> their R-b's for your patch. Chao Zhang, Jiewen, can you please comment? >> >> From my side, dependent on the pending commit message and patch >> whitespace corrections (which I'm willing to implement myself, at push): >> >> Reviewed-by: Laszlo Ersek <ler...@redhat.com> > > Here's the TianoCore BZ that I plan to reference in the updated commit > message, as "further known problems": > > https://bugzilla.tianocore.org/show_bug.cgi?id=1008 > > Thanks > Laszlo > > >>> Thanks, >>> >>> Roman >>> >>>> On Wed, Jul 11, 2018 at 5:05 AM, Laszlo Ersek <ler...@redhat.com> wrote: >>>> >>>> Hi Roman, >>>> >>>>> On 07/11/18 00:51, rba...@gmail.com wrote: >>>>> From: Roman Bacik <roman.ba...@broadcom.com> >>>>> >>>>> When secure boot is enabled, if one loads keys from a FAT formatted >>>>> eMMC/SD/USB when trying to provision PK/KEK/DB keys via the menu, >>>>> an assert in StrLen() occurs. >>>>> This is because the filename starts on odd address, which is not a uint16 >>>>> aligned boundary: https://bugzilla.tianocore.org/show_bug.cgi?id=1003 >>>>> >>>>> Cc: Chao Zhang <chao.b.zh...@intel.com> >>>>> Cc: Jiewen Yao <jiewen....@intel.com> >>>>> Cc: Laszlo Ersek <ler...@redhat.com> >>>>> Cc: Vladimir Olovyannikov <vladimir.olovyanni...@broadcom.com> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Roman Bacik <roman.ba...@broadcom.com> >>>>> --- >>>>> SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c >>>> | 13 +++++++++++-- >>>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> Thank you for sending a well-formed patch. >>>> >>>> I notice that you sent this email from <rba...@gmail.com>, which is not >>>> the same as the Signed-off-by line. I realize you posted from >>>> <rba...@gmail.com> for technical reasons, and it should be no problem. >>>> >>>> However, I *think* in such cases we usually request the following: >>>> >>>> - Using your broadcom.com email address, please respond to this patch >>>> (not my present email, but your original git posting), keeping full >>>> context, and just repeat your Signed-off-by line (referencing the >>>> broadcom address). >>>> >>>> I'm CC'ing Jordan and Ard for confirmation -- I believe this is what >>>> we've done in the past, in cases when submitters had to post their work >>>> from private addresses due to company email issues. >>>> >>>> Technical comments below: >>>> >>>>> diff --git >>>>> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c >>>> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/ >>>> SecureBootConfigFileExplorer.c >>>>> index 1b6f88804275..19b13a5569a6 100644 >>>>> --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/ >>>> SecureBootConfigFileExplorer.c >>>>> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/ >>>> SecureBootConfigFileExplorer.c >>>>> @@ -123,6 +123,8 @@ OpenFileByDevicePath( >>>>> EFI_FILE_PROTOCOL *Handle1; >>>>> EFI_FILE_PROTOCOL *Handle2; >>>>> EFI_HANDLE DeviceHandle; >>>>> + CHAR16 *PathName; >>>>> + UINTN PathLength; >>>>> >>>>> if ((FilePath == NULL || FileHandle == NULL)) { >>>>> return EFI_INVALID_PARAMETER; >>>>> @@ -173,6 +175,11 @@ OpenFileByDevicePath( >>>>> // >>>>> Handle2 = Handle1; >>>>> Handle1 = NULL; >>>>> + PathLength = DevicePathNodeLength(*FilePath) - >>>> sizeof(EFI_DEVICE_PATH_PROTOCOL); >>>>> + PathName = AllocateCopyPool(PathLength, ((FILEPATH_DEVICE_PATH*)* >>>> FilePath)->PathName); >>>> >>>> (1) On both lines above, space characters are missing after: >>>> DevicePathNodeLength, sizeof, and AllocateCopyPool. (Edk2 coding style.) >>>> I think we can fix this up for you when we push the patch. (I'm willing >>>> to help with that, but we need SecurityPkg maintainer review first.) >>>> >>>> >>>>> + if (PathName == NULL) { >>>>> + return EFI_OUT_OF_RESOURCES; >>>>> + } >>>> >>>> (2) I have now reviewed the original state of the function more >>>> carefully, and, while the above "return" branch introduces a leak >>>> *path*, it does not introduce a leak that doesn't already exist! >>>> >>>> In fact, the original function has multiple issues: >>>> >>>> - If the OpenVolume() call fails, "FileHandle" is set to NULL. That's >>>> useless; the intent is obviously to set (*FileHandle) to NULL. >>>> >>>> - At the top of the "while" loop body, "Handle1" stands for an open >>>> EFI_FILE_PROTOCOL. If the device path type check at the top of the loop >>>> body returns EFI_INVALID_PARAMETER, then it (a) performs the same >>>> useless assignment to "FileHandle" as described above, and (b) fails to >>>> close "Handle1". This is why I say that the above EFI_OUT_OF_RESOURCES >>>> branch introduces no new leak, just a new path to the existent leak. >>>> >>>> - The OpenFileByDevicePath() function is duplicated in the following >>>> modules: "NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c", and >>>> "MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskFileExplorer.c". With the >>>> implication that the alignment issue you found affects all three drivers! >>>> >>>> >>>> Roman, I realize this could be more than what you signed up for; so >>>> please pick one: >>>> >>>> (2a) you could submit a patch series: >>>> >>>> * Write a patch that sets (*FilePath) to NULL right after the >>>> (FileHandle==NULL) check, in preparation for failure, and removes all >>>> the bogus FileHandle=NULL assignments. >>>> >>>> * Write another patch that plugs the leak when the device path type >>>> check fails -- introduce a "CloseHandle1" label at the end of the >>>> function, and jump to it when the devpath type check fails, so that we >>>> close "Handle1". This patch should also invert the meanings of Handle2 >>>> and Handle1 -- the reassignment to Handle1 should only occur *after* we >>>> successfully open Handle2. "Handle1" should *always* remain suitable for >>>> closing through the "CloseHandle1" error path. >>>> >>>> * Include your current patch, for fixing the alignment issue. >>>> >>>> * Write another patch that moves the OpenFileByDevicePath() function to >>>> UefiLib in MdePkg -- under the name EfiOpenFileByDevicePath() -- from >>>> SecureBootConfigDxe. >>>> >>>> * write two more patches, namely for TlsAuthConfigDxe and RamDiskDxe, in >>>> order to consume EfiOpenFileByDevicePath() from UefiLib. Both of those >>>> modules already depend on UefiLib. >>>> >>>> (2b) Alternatively: >>>> >>>> * we can report a new TianoCore BZ about the issues I list above, >>>> >>>> * we can commit this patch of yours as-is, just additionally reference >>>> the *new* BZ in the commit message, as "further known issues", >>>> >>>> * I can work on the rest of the issues. >>>> >>>> >>>> If you pick (2b), then I can >>>> - file the new BZ, >>>> - update the commit message for you, >>>> - update the patch for you, as described in (1), >>>> - ACK this patch (as updated above), >>>> - push the patch (if SecurityPkg maintainers agree), >>>> - take on the new BZ as well. >>>> >>>> Thanks! >>>> Laszlo >>>> >>>>> >>>>> // >>>>> // Try to test opening an existing file >>>>> @@ -180,7 +187,7 @@ OpenFileByDevicePath( >>>>> Status = Handle2->Open ( >>>>> Handle2, >>>>> &Handle1, >>>>> - ((FILEPATH_DEVICE_PATH*)*FilePath)->PathName, >>>>> + PathName, >>>>> OpenMode &~EFI_FILE_MODE_CREATE, >>>>> 0 >>>>> ); >>>>> @@ -192,7 +199,7 @@ OpenFileByDevicePath( >>>>> Status = Handle2->Open ( >>>>> Handle2, >>>>> &Handle1, >>>>> - ((FILEPATH_DEVICE_PATH*)* >>>> FilePath)->PathName, >>>>> + PathName, >>>>> OpenMode, >>>>> Attributes >>>>> ); >>>>> @@ -202,6 +209,8 @@ OpenFileByDevicePath( >>>>> // >>>>> Handle2->Close (Handle2); >>>>> >>>>> + FreePool (PathName); >>>>> + >>>>> if (EFI_ERROR(Status)) { >>>>> return (Status); >>>>> } >>>>> >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>> >>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
