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

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

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