On 06/01/15 14:08, Heyi Guo wrote:
> During UEFI SCT, it will throw an exception because "Progress" is
> passed in with NULL and RouteConfig will try to access the string at
> *(EFI_STRING *0), i.e. 0xFFFFFFFF14000400.
>
> Add sanity check for ExtractConfig and RouteConfig to avoid NULL
> pointer dereference.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Heyi Guo <heyi....@linaro.org>
> ---
> OvmfPkg/PlatformDxe/Platform.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
> index 4ec327e..35fabf8 100644
> --- a/OvmfPkg/PlatformDxe/Platform.c
> +++ b/OvmfPkg/PlatformDxe/Platform.c
> @@ -234,6 +234,11 @@ ExtractConfig (
> MAIN_FORM_STATE MainFormState;
> EFI_STATUS Status;
>
> + if (Progress == NULL || Results == NULL)
> + {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> DEBUG ((EFI_D_VERBOSE, "%a: Request=\"%s\"\n", __FUNCTION__, Request));
>
> Status = PlatformConfigToFormState (&MainFormState);
EFI_HII_CONFIG_ROUTING_PROTOCOL.ExtractConfig() does not require any of
these checks. Both Progress and Results are OUT parameters (ie.
pointers) and nothing in the spec resolves the caller from passing in
NULL (or non-NULL garbage, for that matter, which you couldn't catch
anyway).
I can ACK this hunk if you show me a confirmed Mantis ticket for the
UEFI spec.
> @@ -327,6 +332,11 @@ RouteConfig (
> UINTN BlockSize;
> EFI_STATUS Status;
>
> + if (Configuration == NULL || Progress == NULL)
> + {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> DEBUG ((EFI_D_VERBOSE, "%a: Configuration=\"%s\"\n", __FUNCTION__,
> Configuration));
>
>
The (Configuration == NULL) check is valid, according to UEFI-2.5. But,
please update the leading comment on the function accordingly -- please
document the EFI_INVALID_PARAMETER retval.
The (Progress == NULL) check is not required by the spec, and the
responsibility of the caller is made clear for this output parameter:
Progress
A pointer to a string filled in with the offset of the most recent
‘&’ before the first failing name / value pair (or the beginning of
the string if the failure is in the first name / value pair) or the
terminating NULL if all was successful.
"Pointer to a string" implies Progress cannot be NULL. (And, on output,
*Progress will point into Configuration.)
I do understand that suppressing these SCT bugs in OVMF would be easy.
That doesn't make it right. Not NACKing this, but you'll have to get an
ACK from Jordan. (Personally I'm okay only with the
(Configuration==NULL) check in RouteConfig().)
Thanks
Laszlo
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
