Re: [PATCH 3/6] ArmPkg/LinuxLoader: eliminate calls to deprecated string functions

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 25 October 2016 at 12:08, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 25 October 2016 at 11:56, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 25 October 2016 at 11:53, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 10/24/16 18:06, Ard Biesheuvel wrote:
>>>> Remove calls to deprecated string functions like AsciiStrCpy() and
>>>> UnicodeStrToAsciiStr()
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> ---
>>>>  ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c | 2 +-
>>>>  ArmPkg/Application/LinuxLoader/LinuxLoader.c   | 6 ++++--
>>>>  2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c b/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
>>>> index fd7ee9c8624d..0b3e2489c758 100644
>>>> --- a/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
>>>> +++ b/ArmPkg/Application/LinuxLoader/Arm/LinuxAtag.c
>>>> @@ -72,7 +72,7 @@ SetupCmdlineTag (
>>>>      mLinuxKernelCurrentAtag->header.type = ATAG_CMDLINE;
>>>>
>>>>      /* place CommandLine into tag */
>>>> -    AsciiStrCpy (mLinuxKernelCurrentAtag->body.cmdline_tag.cmdline, CmdLine);
>>>> +    AsciiStrCpyS (mLinuxKernelCurrentAtag->body.cmdline_tag.cmdline, LineLength, CmdLine);
>>>>
>>>>      // move pointer to next tag
>>>>      mLinuxKernelCurrentAtag = next_tag_address (mLinuxKernelCurrentAtag);
>>>
>>> Apparently nothing in this file checks if the tags being added still
>>> actually fit in the preallocated space (which is EFI_SIZE_TO_PAGES
>>> (ATAG_MAX_SIZE)). The change does preserve the previous behavior ("copy
>>> the full string").
>>>
>>
>> The LinuxLoader is an unmaintained piece of junk, and will be removed
>> as soon as we can.
>
> Who still uses it? Hikey?
>

I certainly hope not. HiKey is AARCH64, and LinuxLoader is quite badly
broken on that architecture.

>
>> I did notice that none of these ATAG functions
>> check whether the allocation as a whole is not overrun, but fixing
>> /that/ goes way beyond what we're willing to do in terms of
>> maintenance on deprecated code.
>>
>>>> diff --git a/ArmPkg/Application/LinuxLoader/LinuxLoader.c b/ArmPkg/Application/LinuxLoader/LinuxLoader.c
>>>> index 70b960b66f0e..76697c3a8c9d 100644
>>>> --- a/ArmPkg/Application/LinuxLoader/LinuxLoader.c
>>>> +++ b/ArmPkg/Application/LinuxLoader/LinuxLoader.c
>>>> @@ -61,6 +61,7 @@ LinuxLoaderEntryPoint (
>>>>    LIST_ENTRY                          *ResourceLink;
>>>>    SYSTEM_MEMORY_RESOURCE              *Resource;
>>>>    EFI_PHYSICAL_ADDRESS                SystemMemoryBase;
>>>> +  UINTN                               Length;
>>>>
>>>>    Status = gBS->LocateProtocol (
>>>>                    &gEfiDevicePathFromTextProtocolGuid,
>>>> @@ -182,12 +183,13 @@ LinuxLoaderEntryPoint (
>>>>    }
>>>>
>>>>    if (LinuxCommandLine != NULL) {
>>>> -    AsciiLinuxCommandLine = AllocatePool ((StrLen (LinuxCommandLine) + 1) * sizeof (CHAR8));
>>>> +    Length = StrLen (LinuxCommandLine) + 1;
>>>> +    AsciiLinuxCommandLine = AllocatePool (Length);
>>>>      if (AsciiLinuxCommandLine == NULL) {
>>>>        Status = EFI_OUT_OF_RESOURCES;
>>>>        goto Error;
>>>>      }
>>>> -    UnicodeStrToAsciiStr (LinuxCommandLine, AsciiLinuxCommandLine);
>>>> +    UnicodeStrToAsciiStrS (LinuxCommandLine, AsciiLinuxCommandLine, Length);
>>>>    }
>>>>
>>>>    //
>>>>
>>>
>>> I prefer to call character counts without the terminating NUL "Length",
>>> and character counts with the terminating NUL "Size", but that's just a
>>> personal preference. (And the rest of this code uses Length differently
>>> already, for example in "LineLength" itself, near the top.)
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>
>> Thanks,
>> Ard.
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel