From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi0-x233.google.com (mail-oi0-x233.google.com [IPv6:2607:f8b0:4003:c06::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C892D1A1E46 for ; Tue, 25 Oct 2016 03:56:11 -0700 (PDT) Received: by mail-oi0-x233.google.com with SMTP id i127so37092649oia.2 for ; Tue, 25 Oct 2016 03:56:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=R9rZou6r5iHhBHFDSOrAW0dLlQVj7TrYLNaM4r3qp2M=; b=X+cNxx6JHTy2M+WZBy1Pmi2q8qIRfL+Hg6eByOoQRZiIPxVHPnhEET+mm/nT3qdLXR zRxt1YYWHa41wgcuPuZInP6gxPDysyLEF7n6+vPT9HvdjeN67aK9w1KmDRG2u1QOKbGb /uytzdOWTylP/8N7NSN4RRJsyIT/o/1fowoXk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=R9rZou6r5iHhBHFDSOrAW0dLlQVj7TrYLNaM4r3qp2M=; b=kD79gPARRRRSQrCUc4Nkz6D4brbattCm2QOBfdh6+UdD83wG7MjlceHzrizqsn1xvt YAjgYTfrToOFhzIkk0kfiF07OOpg0p4lXeRTNxs0/YFwccaKCbBFd5E34B7zFM5MOLA4 /LNAI08k2ZTECkU2GjVdAKfJJqRvRGWMMU9XdWdeUc/x6/O8Ln9/xVU+/whXT6jUr9/T LIxLUjoTzJwqKs/5W6DhDJMwfFjATwDKC/bZSgAgrHVLyibpNHPpMMS1K4QlzLX3jWJg av3aHCIRUXwxQp2YIpT8d90UUysiGe2gYy2OYSMlJocdCu17Px82iHxEpB5vAR10Aski NftA== X-Gm-Message-State: ABUngvfbfMA8sDeV0T765C0U20jtMpwNHJpkcG5pS56ENqlZrifb0Zj9eWrnC5IVBsS9fJIL2WbQVlsS+0w5SDJl X-Received: by 10.107.28.148 with SMTP id c142mr15056636ioc.45.1477392970875; Tue, 25 Oct 2016 03:56:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.5.139 with HTTP; Tue, 25 Oct 2016 03:56:10 -0700 (PDT) In-Reply-To: <4819b517-89e8-3c61-ebb7-a718ae4be909@redhat.com> References: <1477325206-24646-1-git-send-email-ard.biesheuvel@linaro.org> <1477325206-24646-4-git-send-email-ard.biesheuvel@linaro.org> <4819b517-89e8-3c61-ebb7-a718ae4be909@redhat.com> From: Ard Biesheuvel Date: Tue, 25 Oct 2016 11:56:10 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH 3/6] ArmPkg/LinuxLoader: eliminate calls to deprecated string functions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2016 10:56:12 -0000 Content-Type: text/plain; charset=UTF-8 On 25 October 2016 at 11:53, Laszlo Ersek 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 >> --- >> 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 > Thanks, Ard.