Hi Laszlo, I added the forced conversion just for passing gcc build. I didn't find the root cause of the build failure issue cause current bug.
Hi Gary, Sorry for causing you any inconvenience when using this driver. Thanks, Dandan -----Original Message----- From: Laszlo Ersek [mailto:[email protected]] Sent: Monday, March 21, 2016 4:29 PM To: Gary Lin <[email protected]>; [email protected] Cc: Bi, Dandan <[email protected]> Subject: Re: [edk2] [PATCH] SecurityPkg/SecureBootConfigDxe: Declare EFIAPI for the ChooseFile handlers Hi Gary, On 03/21/16 09:00, Gary Lin wrote: > The SecureBootConfig now uses ChooseFile() from FileExplorerLib to > select the certificates to be enrolled into PK, KEK, DB, DBX, or DBT, > and the corresponding handlers to get the content of the file. Per the > definition of CHOOSE_HANDLER, the handler must use EFIAPI as the > calling convention. However, the calling convention was not specified > the following handlers: UpdatePKFromFile(), UpdateKEKFromFile(), > UpdateDBFromFile(), UpdateDBXFromFile(), and UpdateDBTFromFile(). When > compiling the firmware with gcc, the default calling convention is not > compatible with EFIAPI, so the handlers interpreted the argument the > wrong way and passed the wrong device path to UpdatePage(), and the > system crashed when the user tried to enroll a certificate into the > key database. > > This commit specifies the calling convention for those functions so > that gcc can generate the right code. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Gary Lin <[email protected]> > --- > .../SecureBootConfigDxe/SecureBootConfigFileExplorer.c | 5 > +++++ > .../VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h > | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf > igFileExplorer.c > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf > igFileExplorer.c > index 05d97dc..1b6f888 100644 > --- > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf > igFileExplorer.c > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot > +++ ConfigFileExplorer.c > @@ -343,6 +343,7 @@ UpdatePage( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdatePKFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ) > @@ -360,6 +361,7 @@ UpdatePKFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateKEKFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ) > @@ -376,6 +378,7 @@ UpdateKEKFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateDBFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ) > @@ -392,6 +395,7 @@ UpdateDBFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateDBXFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ) > @@ -408,6 +412,7 @@ UpdateDBXFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateDBTFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ) > diff --git > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf > igImpl.h > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf > igImpl.h > index a8dbd92..1ee9580 100644 > --- > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf > igImpl.h > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot > +++ ConfigImpl.h > @@ -561,6 +561,7 @@ GuidToString ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdatePKFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ); > @@ -574,6 +575,7 @@ UpdatePKFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateKEKFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ); > @@ -587,6 +589,7 @@ UpdateKEKFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateDBFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ); > @@ -600,6 +603,7 @@ UpdateDBFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateDBXFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ); > @@ -613,6 +617,7 @@ UpdateDBXFromFile ( > @retval FALSE Not exit caller function. > **/ > BOOLEAN > +EFIAPI > UpdateDBTFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ); > This patch is good, but it is not sufficient. In every such case the question must be asked, "why does the code compile at all"? The answer is almost always "because the pointer to the non-EFIAPI function undergoes a forced bogus conversion to a pointer to an EFIAPI function". This is exactly the case here. If you grep for the use of these functions: $ git grep -Enw \ 'UpdatePKFromFile|UpdateKEKFromFile|UpdateDBFromFile|UpdateDBXFromFile|UpdateDBTFromFile' you will see that the real bug is in the SecureBootCallback() function, in the file "SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c": case FORMID_ENROLL_PK_FORM: ChooseFile( NULL, NULL, (CHOOSE_HANDLER) UpdatePKFromFile, &File); break; case FORMID_ENROLL_KEK_FORM: ChooseFile( NULL, NULL, (CHOOSE_HANDLER) UpdateKEKFromFile, &File); break; case SECUREBOOT_ENROLL_SIGNATURE_TO_DB: ChooseFile( NULL, NULL, (CHOOSE_HANDLER) UpdateDBFromFile, &File); break; case SECUREBOOT_ENROLL_SIGNATURE_TO_DBX: ChooseFile( NULL, NULL, (CHOOSE_HANDLER) UpdateDBXFromFile, &File); break; case SECUREBOOT_ENROLL_SIGNATURE_TO_DBT: ChooseFile( NULL, NULL, (CHOOSE_HANDLER) UpdateDBTFromFile, &File); break; All those explicit (CHOOSE_HANDLER) casts are bugs. Had they not been there, this EFIAPI bug would have been caught by the compiler, and the file would have never built. The CHOOSE_HANDLER typedef is: typedef BOOLEAN (EFIAPI *CHOOSE_HANDLER)( IN EFI_DEVICE_PATH_PROTOCOL *FilePath ); The pointers to the five functions that you are correcting now were originally incompatible with this typedef, so the compiler would have rejected them as arguments to ChooseFile(). Please append another patch to this series where the above casts are removed. While at it, I recommend to fix up the incorrect paren style in these ChooseFile() calls -- the space should be before the opening parenthesis, not after. Dandan, this code was introduced in commit 762d8ddb2877. May I ask what specific reason you had for casting the pointers? Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
