* [PATCH v1 1/1] NetworkPkg/Ip6Dxe: Validate source data record length
@ 2020-04-08 5:46 Michael Kubacki
2020-04-09 8:22 ` Siyuan, Fu
0 siblings, 1 reply; 3+ messages in thread
From: Michael Kubacki @ 2020-04-08 5:46 UTC (permalink / raw)
To: devel; +Cc: Siyuan Fu, Maciej Rabeda, Jiaxin Wu
From: Michael Kubacki <michael.kubacki@microsoft.com>
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2273
Ip6ConfigReadConfigData() reads configuration data from a UEFI variable
and copies the data to another buffer. This change checks that the length
of the data record being copied does not exceed the size of the source
UEFI variable data buffer.
If the size is exceeded, this change follows existing logic to treat the
variable as corrupted and deletes the variable so it will be set again.
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c | 47 +++++++++++++-------
1 file changed, 30 insertions(+), 17 deletions(-)
diff --git a/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c b/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
index eb2a80b64f15..ab3801336912 100644
--- a/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
+++ b/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
@@ -2,6 +2,7 @@
The implementation of EFI IPv6 Configuration Protocol.
Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -390,24 +391,9 @@ Ip6ConfigReadConfigData (
);
if (EFI_ERROR (Status) || (UINT16) (~NetblockChecksum ((UINT8 *) Variable, (UINT32) VarSize)) != 0) {
//
- // GetVariable still error or the variable is corrupted.
- // Fall back to the default value.
+ // GetVariable error or the variable is corrupted.
//
- FreePool (Variable);
-
- //
- // Remove the problematic variable and return EFI_NOT_FOUND, a new
- // variable will be set again.
- //
- gRT->SetVariable (
- VarName,
- &gEfiIp6ConfigProtocolGuid,
- IP6_CONFIG_VARIABLE_ATTRIBUTE,
- 0,
- NULL
- );
-
- return EFI_NOT_FOUND;
+ goto Error;
}
//
@@ -432,7 +418,12 @@ Ip6ConfigReadConfigData (
if (!DATA_ATTRIB_SET (DataItem->Attribute, DATA_ATTRIB_SIZE_FIXED)) {
//
// This data item has variable length data.
+ // Check that the length is contained within the variable before allocating.
//
+ if (DataRecord.DataSize > VarSize - DataRecord.Offset) {
+ goto Error;
+ }
+
DataItem->Data.Ptr = AllocatePool (DataRecord.DataSize);
if (DataItem->Data.Ptr == NULL) {
//
@@ -454,6 +445,28 @@ Ip6ConfigReadConfigData (
}
return Status;
+
+Error:
+ //
+ // Fall back to the default value.
+ //
+ if (Variable != NULL) {
+ FreePool (Variable);
+ }
+
+ //
+ // Remove the problematic variable and return EFI_NOT_FOUND, a new
+ // variable will be set again.
+ //
+ gRT->SetVariable (
+ VarName,
+ &gEfiIp6ConfigProtocolGuid,
+ IP6_CONFIG_VARIABLE_ATTRIBUTE,
+ 0,
+ NULL
+ );
+
+ return EFI_NOT_FOUND;
}
/**
--
2.16.3.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v1 1/1] NetworkPkg/Ip6Dxe: Validate source data record length
2020-04-08 5:46 [PATCH v1 1/1] NetworkPkg/Ip6Dxe: Validate source data record length Michael Kubacki
@ 2020-04-09 8:22 ` Siyuan, Fu
2020-04-17 15:44 ` [edk2-devel] " Maciej Rabeda
0 siblings, 1 reply; 3+ messages in thread
From: Siyuan, Fu @ 2020-04-09 8:22 UTC (permalink / raw)
To: michael.kubacki@outlook.com, devel@edk2.groups.io
Cc: Maciej Rabeda, Wu, Jiaxin
Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>
> -----Original Message-----
> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
> Sent: 2020年4月8日 13:47
> To: devel@edk2.groups.io
> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Maciej Rabeda
> <maciej.rabeda@linux.intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [PATCH v1 1/1] NetworkPkg/Ip6Dxe: Validate source data record
> length
>
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2273
>
> Ip6ConfigReadConfigData() reads configuration data from a UEFI variable
> and copies the data to another buffer. This change checks that the length
> of the data record being copied does not exceed the size of the source
> UEFI variable data buffer.
>
> If the size is exceeded, this change follows existing logic to treat the
> variable as corrupted and deletes the variable so it will be set again.
>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c | 47 +++++++++++++-------
> 1 file changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
> b/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
> index eb2a80b64f15..ab3801336912 100644
> --- a/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
> +++ b/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
> @@ -2,6 +2,7 @@
> The implementation of EFI IPv6 Configuration Protocol.
>
> Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> + Copyright (c) Microsoft Corporation.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
> @@ -390,24 +391,9 @@ Ip6ConfigReadConfigData (
> );
> if (EFI_ERROR (Status) || (UINT16) (~NetblockChecksum ((UINT8 *)
> Variable, (UINT32) VarSize)) != 0) {
> //
> - // GetVariable still error or the variable is corrupted.
> - // Fall back to the default value.
> + // GetVariable error or the variable is corrupted.
> //
> - FreePool (Variable);
> -
> - //
> - // Remove the problematic variable and return EFI_NOT_FOUND, a new
> - // variable will be set again.
> - //
> - gRT->SetVariable (
> - VarName,
> - &gEfiIp6ConfigProtocolGuid,
> - IP6_CONFIG_VARIABLE_ATTRIBUTE,
> - 0,
> - NULL
> - );
> -
> - return EFI_NOT_FOUND;
> + goto Error;
> }
>
> //
> @@ -432,7 +418,12 @@ Ip6ConfigReadConfigData (
> if (!DATA_ATTRIB_SET (DataItem->Attribute, DATA_ATTRIB_SIZE_FIXED)) {
> //
> // This data item has variable length data.
> + // Check that the length is contained within the variable before
> allocating.
> //
> + if (DataRecord.DataSize > VarSize - DataRecord.Offset) {
> + goto Error;
> + }
> +
> DataItem->Data.Ptr = AllocatePool (DataRecord.DataSize);
> if (DataItem->Data.Ptr == NULL) {
> //
> @@ -454,6 +445,28 @@ Ip6ConfigReadConfigData (
> }
>
> return Status;
> +
> +Error:
> + //
> + // Fall back to the default value.
> + //
> + if (Variable != NULL) {
> + FreePool (Variable);
> + }
> +
> + //
> + // Remove the problematic variable and return EFI_NOT_FOUND, a new
> + // variable will be set again.
> + //
> + gRT->SetVariable (
> + VarName,
> + &gEfiIp6ConfigProtocolGuid,
> + IP6_CONFIG_VARIABLE_ATTRIBUTE,
> + 0,
> + NULL
> + );
> +
> + return EFI_NOT_FOUND;
> }
>
> /**
> --
> 2.16.3.windows.1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [edk2-devel] [PATCH v1 1/1] NetworkPkg/Ip6Dxe: Validate source data record length
2020-04-09 8:22 ` Siyuan, Fu
@ 2020-04-17 15:44 ` Maciej Rabeda
0 siblings, 0 replies; 3+ messages in thread
From: Maciej Rabeda @ 2020-04-17 15:44 UTC (permalink / raw)
To: devel, siyuan.fu, michael.kubacki@outlook.com; +Cc: Wu, Jiaxin
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
On 09-Apr-20 10:22, Siyuan, Fu wrote:
> Reviewed-by: Siyuan Fu <siyuan.fu@intel.com>
>
>> -----Original Message-----
>> From: michael.kubacki@outlook.com <michael.kubacki@outlook.com>
>> Sent: 2020年4月8日 13:47
>> To: devel@edk2.groups.io
>> Cc: Fu, Siyuan <siyuan.fu@intel.com>; Maciej Rabeda
>> <maciej.rabeda@linux.intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
>> Subject: [PATCH v1 1/1] NetworkPkg/Ip6Dxe: Validate source data record
>> length
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2273
>>
>> Ip6ConfigReadConfigData() reads configuration data from a UEFI variable
>> and copies the data to another buffer. This change checks that the length
>> of the data record being copied does not exceed the size of the source
>> UEFI variable data buffer.
>>
>> If the size is exceeded, this change follows existing logic to treat the
>> variable as corrupted and deletes the variable so it will be set again.
>>
>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>> NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c | 47 +++++++++++++-------
>> 1 file changed, 30 insertions(+), 17 deletions(-)
>>
>> diff --git a/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
>> b/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
>> index eb2a80b64f15..ab3801336912 100644
>> --- a/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
>> +++ b/NetworkPkg/Ip6Dxe/Ip6ConfigImpl.c
>> @@ -2,6 +2,7 @@
>> The implementation of EFI IPv6 Configuration Protocol.
>>
>> Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
>> + Copyright (c) Microsoft Corporation.<BR>
>>
>> SPDX-License-Identifier: BSD-2-Clause-Patent
>>
>> @@ -390,24 +391,9 @@ Ip6ConfigReadConfigData (
>> );
>> if (EFI_ERROR (Status) || (UINT16) (~NetblockChecksum ((UINT8 *)
>> Variable, (UINT32) VarSize)) != 0) {
>> //
>> - // GetVariable still error or the variable is corrupted.
>> - // Fall back to the default value.
>> + // GetVariable error or the variable is corrupted.
>> //
>> - FreePool (Variable);
>> -
>> - //
>> - // Remove the problematic variable and return EFI_NOT_FOUND, a new
>> - // variable will be set again.
>> - //
>> - gRT->SetVariable (
>> - VarName,
>> - &gEfiIp6ConfigProtocolGuid,
>> - IP6_CONFIG_VARIABLE_ATTRIBUTE,
>> - 0,
>> - NULL
>> - );
>> -
>> - return EFI_NOT_FOUND;
>> + goto Error;
>> }
>>
>> //
>> @@ -432,7 +418,12 @@ Ip6ConfigReadConfigData (
>> if (!DATA_ATTRIB_SET (DataItem->Attribute, DATA_ATTRIB_SIZE_FIXED)) {
>> //
>> // This data item has variable length data.
>> + // Check that the length is contained within the variable before
>> allocating.
>> //
>> + if (DataRecord.DataSize > VarSize - DataRecord.Offset) {
>> + goto Error;
>> + }
>> +
>> DataItem->Data.Ptr = AllocatePool (DataRecord.DataSize);
>> if (DataItem->Data.Ptr == NULL) {
>> //
>> @@ -454,6 +445,28 @@ Ip6ConfigReadConfigData (
>> }
>>
>> return Status;
>> +
>> +Error:
>> + //
>> + // Fall back to the default value.
>> + //
>> + if (Variable != NULL) {
>> + FreePool (Variable);
>> + }
>> +
>> + //
>> + // Remove the problematic variable and return EFI_NOT_FOUND, a new
>> + // variable will be set again.
>> + //
>> + gRT->SetVariable (
>> + VarName,
>> + &gEfiIp6ConfigProtocolGuid,
>> + IP6_CONFIG_VARIABLE_ATTRIBUTE,
>> + 0,
>> + NULL
>> + );
>> +
>> + return EFI_NOT_FOUND;
>> }
>>
>> /**
>> --
>> 2.16.3.windows.1
>
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-17 15:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-08 5:46 [PATCH v1 1/1] NetworkPkg/Ip6Dxe: Validate source data record length Michael Kubacki
2020-04-09 8:22 ` Siyuan, Fu
2020-04-17 15:44 ` [edk2-devel] " Maciej Rabeda
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox