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/SecureBootConfigFileExplorer.c
>
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> index 05d97dc..1b6f888 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.c
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigFileExplorer.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/SecureBootConfigImpl.h
>
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h
> index a8dbd92..1ee9580 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h
> +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.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
