diff mbox series

[edk2] MdeModulePkg/NonDiscoverable: fix memory override bug

Message ID 1509342472-1688-1-git-send-email-heyi.guo@linaro.org
State Accepted
Commit 710d9e69fae6753a1a826aa18dd37bcadd3e0c3e
Headers show
Series [edk2] MdeModulePkg/NonDiscoverable: fix memory override bug | expand

Commit Message

gary guo Oct. 30, 2017, 5:47 a.m. UTC 
For PciIoPciRead interface, memory prior to Buffer would be written
with zeros if Offset was larger than sizeof (Dev->ConfigSpace), which
would cause serious system exception.

So we add a pre-check branch to avoid memory override.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

---
 .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 5 +++++
 1 file changed, 5 insertions(+)

-- 
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Ard Biesheuvel Oct. 30, 2017, 8:14 a.m. UTC | #1 
On 30 October 2017 at 05:47, Heyi Guo <heyi.guo@linaro.org> wrote:
> For PciIoPciRead interface, memory prior to Buffer would be written

> with zeros if Offset was larger than sizeof (Dev->ConfigSpace), which

> would cause serious system exception.

>

> So we add a pre-check branch to avoid memory override.

>

> Cc: Star Zeng <star.zeng@intel.com>

> Cc: Eric Dong <eric.dong@intel.com>

> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

> Contributed-under: TianoCore Contribution Agreement 1.1

> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>


Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


> ---

>  .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 5 +++++

>  1 file changed, 5 insertions(+)

>

> diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c

> index c836ad6..0e42ae4 100644

> --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c

> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c

> @@ -465,6 +465,11 @@ PciIoPciRead (

>    Address = (UINT8 *)&Dev->ConfigSpace + Offset;

>    Length = Count << ((UINTN)Width & 0x3);

>

> +  if (Offset >= sizeof (Dev->ConfigSpace)) {

> +    ZeroMem (Buffer, Length);

> +    return EFI_SUCCESS;

> +  }

> +

>    if (Offset + Length > sizeof (Dev->ConfigSpace)) {

>      //

>      // Read all zeroes for config space accesses beyond the first

> --

> 1.9.1

>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Zeng, Star Oct. 30, 2017, 10:07 a.m. UTC | #2 
Ray,
Please help take a review to this patch.


Thanks,
Star
-----Original Message-----
From: Heyi Guo [mailto:heyi.guo@linaro.org] 

Sent: Monday, October 30, 2017 1:48 PM
To: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org
Cc: Heyi Guo <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>
Subject: [edk2][PATCH] MdeModulePkg/NonDiscoverable: fix memory override bug

For PciIoPciRead interface, memory prior to Buffer would be written with zeros if Offset was larger than sizeof (Dev->ConfigSpace), which would cause serious system exception.

So we add a pre-check branch to avoid memory override.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

---
 .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
index c836ad6..0e42ae4 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePc
+++ iDeviceIo.c
@@ -465,6 +465,11 @@ PciIoPciRead (
   Address = (UINT8 *)&Dev->ConfigSpace + Offset;
   Length = Count << ((UINTN)Width & 0x3);
 
+  if (Offset >= sizeof (Dev->ConfigSpace)) {
+    ZeroMem (Buffer, Length);
+    return EFI_SUCCESS;
+  }
+
   if (Offset + Length > sizeof (Dev->ConfigSpace)) {
     //
     // Read all zeroes for config space accesses beyond the first
--
1.9.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo Oct. 30, 2017, 2:42 p.m. UTC | #3 
Hi Ray,

It seems Ard already provided his R-B :)

Thanks.

Heyi


On 10/30/2017 06:23 PM, Ni, Ruiyu wrote:
> I will wait for Ard's feedback. It's an ARM specific module.

>

> Thanks/Ray

>

>> -----Original Message-----

>> From: Zeng, Star

>> Sent: Monday, October 30, 2017 6:07 PM

>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Heyi Guo <heyi.guo@linaro.org>; linaro-

>> uefi@lists.linaro.org; edk2-devel@lists.01.org

>> Cc: Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel

>> <ard.biesheuvel@linaro.org>; Zeng, Star <star.zeng@intel.com>

>> Subject: RE: [edk2][PATCH] MdeModulePkg/NonDiscoverable: fix memory

>> override bug

>>

>> Ray,

>> Please help take a review to this patch.

>>

>>

>> Thanks,

>> Star

>> -----Original Message-----

>> From: Heyi Guo [mailto:heyi.guo@linaro.org]

>> Sent: Monday, October 30, 2017 1:48 PM

>> To: linaro-uefi@lists.linaro.org; edk2-devel@lists.01.org

>> Cc: Heyi Guo <heyi.guo@linaro.org>; Zeng, Star <star.zeng@intel.com>;

>> Dong, Eric <eric.dong@intel.com>; Ard Biesheuvel

>> <ard.biesheuvel@linaro.org>; Ni, Ruiyu <ruiyu.ni@intel.com>

>> Subject: [edk2][PATCH] MdeModulePkg/NonDiscoverable: fix memory

>> override bug

>>

>> For PciIoPciRead interface, memory prior to Buffer would be written with

>> zeros if Offset was larger than sizeof (Dev->ConfigSpace), which would cause

>> serious system exception.

>>

>> So we add a pre-check branch to avoid memory override.

>>

>> Cc: Star Zeng <star.zeng@intel.com>

>> Cc: Eric Dong <eric.dong@intel.com>

>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

>> ---

>>   .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 5

>> +++++

>>   1 file changed, 5 insertions(+)

>>

>> diff --git

>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable

>> PciDeviceIo.c

>> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable

>> PciDeviceIo.c

>> index c836ad6..0e42ae4 100644

>> ---

>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable

>> PciDeviceIo.c

>> +++

>> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverable

>> Pc

>> +++ iDeviceIo.c

>> @@ -465,6 +465,11 @@ PciIoPciRead (

>>     Address = (UINT8 *)&Dev->ConfigSpace + Offset;

>>     Length = Count << ((UINTN)Width & 0x3);

>>

>> +  if (Offset >= sizeof (Dev->ConfigSpace)) {

>> +    ZeroMem (Buffer, Length);

>> +    return EFI_SUCCESS;

>> +  }

>> +

>>     if (Offset + Length > sizeof (Dev->ConfigSpace)) {

>>       //

>>       // Read all zeroes for config space accesses beyond the first

>> --

>> 1.9.1


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo Nov. 7, 2017, 9:33 a.m. UTC | #4 
Hi Ray,

We had Ard's R-B already; could you help to commit it?

Thanks and regards,

Heyi


在 10/30/2017 4:14 PM, Ard Biesheuvel 写道:
> On 30 October 2017 at 05:47, Heyi Guo <heyi.guo@linaro.org> wrote:
>> For PciIoPciRead interface, memory prior to Buffer would be written
>> with zeros if Offset was larger than sizeof (Dev->ConfigSpace), which
>> would cause serious system exception.
>>
>> So we add a pre-check branch to avoid memory override.
>>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
>> ---
>>   .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
>> index c836ad6..0e42ae4 100644
>> --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
>> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
>> @@ -465,6 +465,11 @@ PciIoPciRead (
>>     Address = (UINT8 *)&Dev->ConfigSpace + Offset;
>>     Length = Count << ((UINTN)Width & 0x3);
>>
>> +  if (Offset >= sizeof (Dev->ConfigSpace)) {
>> +    ZeroMem (Buffer, Length);
>> +    return EFI_SUCCESS;
>> +  }
>> +
>>     if (Offset + Length > sizeof (Dev->ConfigSpace)) {
>>       //
>>       // Read all zeroes for config space accesses beyond the first
>> --
>> 1.9.1
>>
Zeng, Star Nov. 8, 2017, 4:53 a.m. UTC | #5 
Just pushed at 710d9e69fae6753a1a826aa18dd37bcadd3e0c3e.

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Heyi Guo

Sent: Tuesday, November 7, 2017 5:33 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; linaro-uefi <linaro-uefi@lists.linaro.org>
Subject: Re: [edk2] [PATCH] MdeModulePkg/NonDiscoverable: fix memory override bug

Hi Ray,

We had Ard's R-B already; could you help to commit it?

Thanks and regards,

Heyi


在 10/30/2017 4:14 PM, Ard Biesheuvel 写道:
> On 30 October 2017 at 05:47, Heyi Guo <heyi.guo@linaro.org> wrote:

>> For PciIoPciRead interface, memory prior to Buffer would be written 

>> with zeros if Offset was larger than sizeof (Dev->ConfigSpace), which 

>> would cause serious system exception.

>>

>> So we add a pre-check branch to avoid memory override.

>>

>> Cc: Star Zeng <star.zeng@intel.com>

>> Cc: Eric Dong <eric.dong@intel.com>

>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>

>> Contributed-under: TianoCore Contribution Agreement 1.1

>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>

> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

>> ---

>>   .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 5 +++++

>>   1 file changed, 5 insertions(+)

>>

>> diff --git 

>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci

>> DeviceIo.c 

>> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci

>> DeviceIo.c

>> index c836ad6..0e42ae4 100644

>> --- 

>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci

>> DeviceIo.c

>> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverabl

>> +++ ePciDeviceIo.c

>> @@ -465,6 +465,11 @@ PciIoPciRead (

>>     Address = (UINT8 *)&Dev->ConfigSpace + Offset;

>>     Length = Count << ((UINTN)Width & 0x3);

>>

>> +  if (Offset >= sizeof (Dev->ConfigSpace)) {

>> +    ZeroMem (Buffer, Length);

>> +    return EFI_SUCCESS;

>> +  }

>> +

>>     if (Offset + Length > sizeof (Dev->ConfigSpace)) {

>>       //

>>       // Read all zeroes for config space accesses beyond the first

>> --

>> 1.9.1

>>


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
gary guo Nov. 8, 2017, 5:02 a.m. UTC | #6 
Thanks,

Heyi


在 11/8/2017 12:53 PM, Zeng, Star 写道:
> Just pushed at 710d9e69fae6753a1a826aa18dd37bcadd3e0c3e.
>
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Heyi Guo
> Sent: Tuesday, November 7, 2017 5:33 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; linaro-uefi <linaro-uefi@lists.linaro.org>
> Subject: Re: [edk2] [PATCH] MdeModulePkg/NonDiscoverable: fix memory override bug
>
> Hi Ray,
>
> We had Ard's R-B already; could you help to commit it?
>
> Thanks and regards,
>
> Heyi
>
>
> 在 10/30/2017 4:14 PM, Ard Biesheuvel 写道:
>> On 30 October 2017 at 05:47, Heyi Guo <heyi.guo@linaro.org> wrote:
>>> For PciIoPciRead interface, memory prior to Buffer would be written
>>> with zeros if Offset was larger than sizeof (Dev->ConfigSpace), which
>>> would cause serious system exception.
>>>
>>> So we add a pre-check branch to avoid memory override.
>>>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>>> ---
>>>    .../Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c | 5 +++++
>>>    1 file changed, 5 insertions(+)
>>>
>>> diff --git
>>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
>>> DeviceIo.c
>>> b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
>>> DeviceIo.c
>>> index c836ad6..0e42ae4 100644
>>> ---
>>> a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePci
>>> DeviceIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverabl
>>> +++ ePciDeviceIo.c
>>> @@ -465,6 +465,11 @@ PciIoPciRead (
>>>      Address = (UINT8 *)&Dev->ConfigSpace + Offset;
>>>      Length = Count << ((UINTN)Width & 0x3);
>>>
>>> +  if (Offset >= sizeof (Dev->ConfigSpace)) {
>>> +    ZeroMem (Buffer, Length);
>>> +    return EFI_SUCCESS;
>>> +  }
>>> +
>>>      if (Offset + Length > sizeof (Dev->ConfigSpace)) {
>>>        //
>>>        // Read all zeroes for config space accesses beyond the first
>>> --
>>> 1.9.1
>>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
index c836ad6..0e42ae4 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
@@ -465,6 +465,11 @@  PciIoPciRead (
   Address = (UINT8 *)&Dev->ConfigSpace + Offset;
   Length = Count << ((UINTN)Width & 0x3);
 
+  if (Offset >= sizeof (Dev->ConfigSpace)) {
+    ZeroMem (Buffer, Length);
+    return EFI_SUCCESS;
+  }
+
   if (Offset + Length > sizeof (Dev->ConfigSpace)) {
     //
     // Read all zeroes for config space accesses beyond the first