[PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[Heinrich Schuchardt]

Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3029

Function DevicePathToTextConvertDeviceNodeToTextCoverageTest() tests if
DeviceNodeToText() correctly converts a Relative Offset Range node. After
calling SctConvertTextToDeviceNode() it tries to set the field Reserved
of the returned device node to 0.

If the tested firmware does not return the expected text
SctConvertTextToDeviceNode() may return NULL or a device node that is
shorter than expected. In both cases it is not possible to access the
field Reserved.

So we must check both that the returned node is not NULL and that it has
the exepected size.

Due to the missing check a NULL dereference was observed when running the
SCT on U-Boot.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 .../BlackBoxTest/DevicePathToTextBBTestCoverage.c         | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/BlackBoxTest/DevicePathToTextBBTestCoverage.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/BlackBoxTest/DevicePathToTextBBTestCoverage.c
index ee91bdfb..784d4748 100644
--- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/BlackBoxTest/DevicePathToTextBBTestCoverage.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/BlackBoxTest/DevicePathToTextBBTestCoverage.c
@@ -1198,8 +1198,12 @@ DevicePathToTextConvertDeviceNodeToTextCoverageTest (
   ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode1)->EndingOffset = 0x1234;
   Text = DevicePathToText->ConvertDeviceNodeToText (pDeviceNode1, FALSE, FALSE);
   pDeviceNode2 = SctConvertTextToDeviceNode(Text);
-  ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode2)->Reserved = 0;
-
+  SctPrint(L"pDeviceNode2 = %p\n", pDeviceNode2);
+  if (pDeviceNode2 &&
+      ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode2)->Length ==
+      sizeof(MEDIA_OFFSET_DEVICE_PATH)) {
+    ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode2)->Reserved = 0;
+  }
   if ((pDeviceNode2 != NULL) && (SctCompareMem (pDeviceNode2, pDeviceNode1, SctDevicePathNodeLength(pDeviceNode1)) == 0)) {
     AssertionType = EFI_TEST_ASSERTION_PASSED;
   } else {
-- 
2.28.0

Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[G Edhaya Chandran]

[-- Attachment #1: Type: text/plain, Size: 57 bytes --]

Reviewed-by: G Edhaya Chandran<edhaya.chandran@arm.com>

[-- Attachment #2: Type: text/html, Size: 492 bytes --]

Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[Grant Likely]

[-- Attachment #1: Type: text/plain, Size: 71 bytes --]

Looks good to me.

Reviewed-by: Grant Likely <grant.likely@arm.com>

[-- Attachment #2: Type: text/html, Size: 85 bytes --]

Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[G Edhaya Chandran]

[-- Attachment #1: Type: text/plain, Size: 115 bytes --]

Upstreamed by commit-id : https://github.com/tianocore/edk2-test/commit/75c92b85bf9bb82a7049bf44f40a2741f963230c

[-- Attachment #2: Type: text/html, Size: 119 bytes --]

Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[Heinrich Schuchardt]

On 11/24/20 5:07 PM, G Edhaya Chandran wrote:
> Upstreamed by commit-id
> : https://github.com/tianocore/edk2-test/commit/75c92b85bf9bb82a7049bf44f40a2741f963230c
>

Hello Edhaya,

it seems a SctPrint() statement slipped into the patch which was only
used for debugging.

Should I open a new issue? Or just send a patch removing the SctPrint
with reference to #3029?

Best regards

Heinrich

Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[G Edhaya Chandran]

Hi Heinrich,

  Oh okay. Actually, I have already upstreamed this.
I suggest, I will remove the debug SctPrint myself as part of another upstream.

Is it okay?

With Warm Regards,
Edhay


> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Sent: 24 November 2020 21:57
> To: G Edhaya Chandran <Edhaya.Chandran@arm.com>; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in
> DevicePathToText test
>
> On 11/24/20 5:07 PM, G Edhaya Chandran wrote:
> > Upstreamed by commit-id
> > :
> > https://github.com/tianocore/edk2-test/commit/75c92b85bf9bb82a7049bf44
> > f40a2741f963230c
> >
>
> Hello Edhaya,
>
> it seems a SctPrint() statement slipped into the patch which was only used for
> debugging.
>
> Should I open a new issue? Or just send a patch removing the SctPrint with
> reference to #3029?
>
> Best regards
>
> Heinrich
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[Heinrich Schuchardt]

On 11/24/20 5:33 PM, G Edhaya Chandran wrote:
> Hi Heinrich,
>
>    Oh okay. Actually, I have already upstreamed this.
> I suggest, I will remove the debug SctPrint myself as part of another upstream.
>
> Is it okay?
>
> With Warm Regards,
> Edhay

Thanks for taking care.

Best regards

Heinrich

>
>
>> -----Original Message-----
>> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Sent: 24 November 2020 21:57
>> To: G Edhaya Chandran <Edhaya.Chandran@arm.com>; devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in
>> DevicePathToText test
>>
>> On 11/24/20 5:07 PM, G Edhaya Chandran wrote:
>>> Upstreamed by commit-id
>>> :
>>> https://github.com/tianocore/edk2-test/commit/75c92b85bf9bb82a7049bf44
>>> f40a2741f963230c
>>>
>>
>> Hello Edhaya,
>>
>> it seems a SctPrint() statement slipped into the patch which was only used for
>> debugging.
>>
>> Should I open a new issue? Or just send a patch removing the SctPrint with
>> reference to #3029?
>>
>> Best regards
>>
>> Heinrich
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>

Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in DevicePathToText test

[G Edhaya Chandran]

[-- Attachment #1.1: Type: text/plain, Size: 4257 bytes --]

Hi Heinrich,



   This part is giving a build error, can you please check.



https://github.com/tianocore/edk2-test/commit/75c92b85bf9bb82a7049bf44f40a2741f963230c#diff-7f0df1676a2f898b39de5685efb7a296a7f08e5e5ccb4aaec0a5fdebea926727



[cid:image001.jpg@01D6C323.F7844200]





/home/zubmoh01/Work/sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/BlackBoxTest/DevicePathToTextBBTestCoverage.c: In function ‘DevicePathToTextConvertDeviceNodeToTextCoverageTest’:
/home/zubmoh01/Work/sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/BlackBoxTest/DevicePathToTextBBTestCoverage.c:1203:49: error: ‘MEDIA_OFFSET_DEVICE_PATH {​​​​​​aka struct <anonymous>}​​​​​​’ has no member named ‘Length’
       ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode2)->Length ==
                                                 ^~
"aarch64-linux-gnu-gcc" -MMD -MF /home/zubmoh01/Work/sct/Build/UefiSct/RELEASE_GCC5/AARCH64/SctPkg/TestCase/UEFI/EFI/BootServices/ImageServices/BlackBoxTest/Dependency/CombinationImage2/CombinationImage2/OUTPUT/ProtocolDefinition.obj.deps   -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=ImageServices_CombinationImage2Strings -g -Os -fshort-wchar -fno-builtin -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-common -mlittle-endian -fno-short-enums -fverbose-asm -funsigned-char -ffunction-sections -fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-pic -fno-pie -ffixed-x18 -mcmodel=small -flto -Wno-unused-but-set-variable -Wno-unused-const-variable -D EFIAARCH64 -D EFIAARCH64 -ffreestanding -nostdinc -nostdlib -Wno-error=unused-function -Wno-error=unused-but-set-variable -Wno-error -DMDEPKG_NDEBUG -c -o







With Warm Regards,
Edhay







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

> From: Heinrich Schuchardt <xypron.glpk@gmx.de>

> Sent: 24 November 2020 22:38

> To: G Edhaya Chandran <Edhaya.Chandran@arm.com>; devel@edk2.groups.io

> Subject: Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in

> DevicePathToText test

>

> On 11/24/20 5:33 PM, G Edhaya Chandran wrote:

> > Hi Heinrich,

> >

> >    Oh okay. Actually, I have already upstreamed this.

> > I suggest, I will remove the debug SctPrint myself as part of another upstream.

> >

> > Is it okay?

> >

> > With Warm Regards,

> > Edhay

>

> Thanks for taking care.

>

> Best regards

>

> Heinrich

>

> >

> >

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

> >> From: Heinrich Schuchardt <xypron.glpk@gmx.de<mailto:xypron.glpk@gmx.de>>

> >> Sent: 24 November 2020 21:57

> >> To: G Edhaya Chandran <Edhaya.Chandran@arm.com<mailto:Edhaya.Chandran@arm.com>>;

> devel@edk2.groups.io<mailto:devel@edk2.groups.io>

> >> Subject: Re: [edk2-devel] [PATCH 1/1] uefi-sct/SctPkg: NULL deref in

> >> DevicePathToText test

> >>

> >> On 11/24/20 5:07 PM, G Edhaya Chandran wrote:

> >>> Upstreamed by commit-id

> >>> :

> >>> https://github.com/tianocore/edk2-test/commit/75c92b85bf9bb82a7049bf

> >>> 44

> >>> f40a2741f963230c

> >>>

> >>

> >> Hello Edhaya,

> >>

> >> it seems a SctPrint() statement slipped into the patch which was only

> >> used for debugging.

> >>

> >> Should I open a new issue? Or just send a patch removing the SctPrint

> >> with reference to #3029?

> >>

> >> Best regards

> >>

> >> Heinrich

> > IMPORTANT NOTICE: The contents of this email and any attachments are

> confidential and may also be privileged. If you are not the intended recipient,

> please notify the sender immediately and do not disclose the contents to any

> other person, use it for any purpose, or store or copy the information in any

> medium. Thank you.

> >



IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[-- Attachment #1.2: Type: text/html, Size: 10874 bytes --]

[-- Attachment #2: image001.jpg --]
[-- Type: image/jpeg, Size: 29665 bytes --]