Hello Charles,
On 2/24/24 17:01, Charles Hyde wrote:
> There is a bug in the following source file:
>
> uefi-sct\
> SctPkg\
> TestCase\
> UEFI\
> EFI\
> Protocol\
> DevicePathFromText\
> BlackBoxTest\
> DevicePathFromTextBBTestCoverage.c
>
>
> Lines 1737, 1738, 1742, and 1743 reference DNS without any cast
> override, which causes the pointer reference logic to be wrong,
> leading the functions SctStrToIPv4Addr and SctStrToIPv6Addr to cause
> memory corruption. The DNS reference needs to have (UINT8 *) cast
> override, like is done in DevicePathToTextBBTestMain.c.
You are correct, as far as I can tell.
This is a regression from:
commit 847e0363e846296881c238dc43766fd40f6c2aec
Author: Abdul Lateef Attar <abdattar@>
Date: Thu Jun 9 16:24:02 2022 +0530
SctPkg: Fix the UefiSct -Wincompatible-pointer-types warnings
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2126
Fixes the incompatible pointer types warning for UefiSct package.
Cc: G Edhaya Chandran <Edhaya.Chandran@...>
Cc: Barton Gao <gaojie@...>
Cc: Carolyn Gjertsen <Carolyn.Gjertsen@...>
Cc: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@...>
Cc: Eric Jin <eric.jin@...>
Cc: Arvin Chen <arvinx.chen@...>
Cc: Supreeth Venkatesh <Supreeth.Venkatesh@...>
Signed-off-by: Abdul Lateef Attar <abdattar@...>
500 files changed, 5537 insertions(+), 1162 deletions(-)
Unfortunately, the change for "DevicePathFromTextBBTestCoverage.c" in
this -- *way* oversized -- patch was incorrect:
> @@ -1734,13 +1734,13 @@ CreateDNSDeviceNode (
> }
>
> if (DNS->IsIPv6 == 0) {
> - SctStrToIPv4Addr (&IpStr1, (UINT8 *)DNS + sizeof (DNS_DEVICE_PATH));
> - SctStrToIPv4Addr (&IpStr2, (UINT8 *)DNS + sizeof (DNS_DEVICE_PATH) +
> sizeof(EFI_IP_ADDRESS));
> + SctStrToIPv4Addr (&IpStr1, (EFI_IPv4_ADDRESS *)(DNS + sizeof
> (DNS_DEVICE_PATH)));
> + SctStrToIPv4Addr (&IpStr2, (EFI_IPv4_ADDRESS *)(DNS + sizeof
> (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS)));
> }
>
> if (DNS->IsIPv6 == 1) {
> - SctStrToIPv6Addr (&IpStr1, (UINT8 *)DNS + sizeof (DNS_DEVICE_PATH));
> - SctStrToIPv6Addr (&IpStr2, (UINT8 *)DNS + sizeof (DNS_DEVICE_PATH) +
> sizeof(EFI_IP_ADDRESS));
> + SctStrToIPv6Addr (&IpStr1, (EFI_IPv6_ADDRESS *)(DNS + sizeof
> (DNS_DEVICE_PATH)));
> + SctStrToIPv6Addr (&IpStr2, (EFI_IPv6_ADDRESS *)(DNS + sizeof
> (DNS_DEVICE_PATH) + sizeof(EFI_IP_ADDRESS)));
> }
>
> return (EFI_DEVICE_PATH_PROTOCOL *) DNS;
The original (UINT8*) cast was correct for determining the "IPv4Addr"
and "IPv6Addr" argument *values* for SctStrToIPv4Addr() and
SctStrToIPv6Addr(), respectively; the problem was with the argument
*types*. The patch added the necessary outer cast, but incorrectly
removed the still needed inner cast.
... The original code was in poor style to begin with. The following
should work, without any casts at all:
SctStrToIPv4Addr (&IpStr1, &DNS->DnsServerIp[0].v4);
SctStrToIPv4Addr (&IpStr2, &DNS->DnsServerIp[1].v4);
SctStrToIPv6Addr (&IpStr1, &DNS->DnsServerIp[0].v6);
SctStrToIPv6Addr (&IpStr2, &DNS->DnsServerIp[1].v6);
That's because DNS is of type pointer-to-DNS_DEVICE_PATH, and
DNS_DEVICE_PATH is defined as follows, in
"MdePkg/Include/Protocol/DevicePath.h":
> #pragma pack(1)
> ...
> typedef struct {
> EFI_DEVICE_PATH_PROTOCOL Header;
> ///
> /// Indicates the DNS server address is IPv4 or IPv6 address.
> ///
> UINT8 IsIPv6;
> ///
> /// Instance of the DNS server address.
> ///
> EFI_IP_ADDRESS DnsServerIp[];
> } DNS_DEVICE_PATH;
> ...
> #pragma pack()
and "EFI_IP_ADDRESS" is defined in "MdePkg/Include/Uefi/UefiBaseType.h"
as follows:
> typedef union {
> UINT32 Addr[4];
> EFI_IPv4_ADDRESS v4;
> EFI_IPv6_ADDRESS v6;
> } EFI_IP_ADDRESS;
The "DnsServerIp" flexible array member should simply be put to use.
AbduL, can you please post a fix?
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115930): https://edk2.groups.io/g/devel/message/115930
Mute This Topic: https://groups.io/mt/104548292/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
