Re: [edk2-devel] edk2-test: bug in DevicePathFromTextBBTestCoverage.c

["Laszlo Ersek" <lersek@redhat.com>]

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-