On 06/01/15 16:14, Laszlo Ersek wrote:
> 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.
Sorry, I just noticed that above I referenced
EFI_HII_CONFIG_ROUTING_PROTOCOL rather than EFI_HII_CONFIG_ACCESS_PROTOCOL.
However, after checking EFI_HII_CONFIG_ACCESS_PROTOCOL.ExtractConfig()
specifically, I still can't see any requirement for these checks.
>> @@ -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().)
Again, I looked at the wrong part of the spec, sorry about that.
But, EFI_HII_CONFIG_ACCESS_PROTOCOL.RouteConfig() doesn't even require a
nullity check for Configuration. (It speaks about "Results", which
doesn't exist as a parameter in that function -- this is a spec bug.)
Please ask someone else to give his/her R-b, because I don't think I will.
Thanks
Laszlo
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
