diff mbox

[edk2,3/6] ArmPkg/LinuxLoader: eliminate calls to deprecated string functions

Message ID 1477325206-24646-4-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show

Commit Message

Ard Biesheuvel Oct. 24, 2016, 4:06 p.m. UTC 
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(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Oct. 25, 2016, 10:53 a.m. UTC | #1 
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").

> 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
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 25, 2016, 10:56 a.m. UTC | #2 
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.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryan Harkin Oct. 25, 2016, 11:08 a.m. UTC | #3 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Oct. 25, 2016, 11:09 a.m. UTC | #4 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

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);
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);
   }
 
   //