Topics

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


Heinrich Schuchardt
 

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

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>
---
.../BlackBoxTest/DevicePathToTextBBTestCoverage.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/Bl=
ackBoxTest/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/BlackBoxT=
est/DevicePathToTextBBTestCoverage.c
+++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/Protocol/DevicePathToText/BlackBoxT=
est/DevicePathToTextBBTestCoverage.c
@@ -1198,8 +1198,12 @@ DevicePathToTextConvertDeviceNodeToTextCoverageTest (
((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode1)->EndingOffset =3D 0x1234;=0D
Text =3D DevicePathToText->ConvertDeviceNodeToText (pDeviceNode1, FALSE,=
FALSE);=0D
pDeviceNode2 =3D SctConvertTextToDeviceNode(Text);=0D
- ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode2)->Reserved =3D 0;=0D
-=0D
+ SctPrint(L"pDeviceNode2 =3D %p\n", pDeviceNode2);=0D
+ if (pDeviceNode2 &&=0D
+ ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode2)->Length =3D=3D=0D
+ sizeof(MEDIA_OFFSET_DEVICE_PATH)) {=0D
+ ((MEDIA_OFFSET_DEVICE_PATH *)pDeviceNode2)->Reserved =3D 0;=0D
+ }=0D
if ((pDeviceNode2 !=3D NULL) && (SctCompareMem (pDeviceNode2, pDeviceNod=
e1, SctDevicePathNodeLength(pDeviceNode1)) =3D=3D 0)) {=0D
AssertionType =3D EFI_TEST_ASSERTION_PASSED;=0D
} else {=0D
--=20
2.28.0


G Edhaya Chandran
 

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


Grant Likely
 

Looks good to me.

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


G Edhaya Chandran
 

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


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


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>
Sent: 24 November 2020 21:57
To: G Edhaya Chandran <@edhay>; 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.


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>
Sent: 24 November 2020 21:57
To: G Edhaya Chandran <@edhay>; 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.


G Edhaya Chandran
 

Hi Heinrich,

 

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

 

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

 

 

 

/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@...>

> Sent: 24 November 2020 22:38

> To: G Edhaya Chandran <Edhaya.Chandran@...>; 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@...>

> >> Sent: 24 November 2020 21:57

> >> To: G Edhaya Chandran <Edhaya.Chandran@...>;

> 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.