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

[Ryan Harkin <ryan.harkin@linaro.org>]

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