From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-x229.google.com (mail-lf0-x229.google.com [IPv6:2a00:1450:4010:c07::229]) (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 BD85E1A1E43 for ; Wed, 26 Oct 2016 05:24:31 -0700 (PDT) Received: by mail-lf0-x229.google.com with SMTP id m193so5169499lfm.4 for ; Wed, 26 Oct 2016 05:24:31 -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=p7fWK4RKQAPiwGahOqTwtnECh9RMZR1wgJc7uecf8yY=; b=Zx3um8uXiT/zjswpYGuXgx3FMQFtvnXPepEqHV1UD2sGeD84mVSm9zeIRfjV9zmePQ wl8Azq+5WwT6TNon1vLkXxVkcDTY7W3BWWKi+WzSHUYSxmK4tHnNu0yBFKm0mFh298sm w5Fn3nvVqSEl4/Hr700xyQXhwAsjWJ8wsjHUI= 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=p7fWK4RKQAPiwGahOqTwtnECh9RMZR1wgJc7uecf8yY=; b=maHSmy602BdQ8I4mVOTuVqklNz93mcXcEGOe2VC+5o0FUKZBtIQHK6PRla+Pl59snC NoDL4ZvK7E5cRemy6Mvj144vn98IMpubS8R1DoG2GgyY9+rG38jmILkgwGVVRsRIr89r DMhstnqqapzoz5Mk/WkM3Iu0xI8ptLQyy61nuEQtj6mOg1Z4bPp4C6+xSoh61urYfU2o kJKnc2eW9L1rr1a61SBIMBuYO8B4nfmTLxXE64QGP6sLubsIZ6EXU/xUaMSRc5sMMKMg MIGSYFHJPUDgWpdvvTs77TUzYc/V/W84p0ZNwskbqxzaFUDXF6AiV065oUsZUTu8SxEO UwHA== X-Gm-Message-State: ABUngvclEjMWnike9yiS5QPdvXXzPpP6FRi/UrJqK+ge9e6g7Kw57AcrzO+bPH/RdY1pih7covmLh1q2Z+iFNm00 X-Received: by 10.25.153.75 with SMTP id b72mr1156450lfe.112.1477484669907; Wed, 26 Oct 2016 05:24:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.28.78 with HTTP; Wed, 26 Oct 2016 05:24:29 -0700 (PDT) In-Reply-To: References: <1477469862-10046-1-git-send-email-ard.biesheuvel@linaro.org> <1477469862-10046-5-git-send-email-ard.biesheuvel@linaro.org> From: Ryan Harkin Date: Wed, 26 Oct 2016 13:24:29 +0100 Message-ID: To: Laszlo Ersek Cc: Ard Biesheuvel , "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH v2 4/6] ArmPkg/SemihostFs: 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: Wed, 26 Oct 2016 12:24:32 -0000 Content-Type: text/plain; charset=UTF-8 On 26 October 2016 at 12:55, Laszlo Ersek wrote: > On 10/26/16 10:17, 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/Filesystem/SemihostFs/Arm/SemihostFs.c | 20 ++++++++++++-------- >> 1 file changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c >> index 6efdad9ebcce..cf94ecd5d56f 100644 >> --- a/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c >> +++ b/ArmPkg/Filesystem/SemihostFs/Arm/SemihostFs.c >> @@ -207,11 +207,12 @@ FileOpen ( >> return EFI_WRITE_PROTECTED; >> } >> >> - AsciiFileName = AllocatePool (StrLen (FileName) + 1); >> + Length = StrLen (FileName) + 1; >> + AsciiFileName = AllocatePool (Length); >> if (AsciiFileName == NULL) { >> return EFI_OUT_OF_RESOURCES; >> } >> - UnicodeStrToAsciiStr (FileName, AsciiFileName); >> + UnicodeStrToAsciiStrS (FileName, AsciiFileName, Length); >> >> // Opening '/', '\', '.', or the NULL pathname is trying to open the root directory >> if ((AsciiStrCmp (AsciiFileName, "\\") == 0) || >> @@ -463,7 +464,7 @@ FileDelete ( >> NameSize = AsciiStrLen (Fcb->FileName); >> FileName = AllocatePool (NameSize + 1); >> >> - AsciiStrCpy (FileName, Fcb->FileName); >> + AsciiStrCpyS (FileName, NameSize + 1, Fcb->FileName); >> >> // Close the file if it's open. Disregard return status, >> // since it might give an error if the file isn't open. >> @@ -828,8 +829,10 @@ GetFilesystemInfo ( >> EFI_FILE_SYSTEM_INFO *Info; >> EFI_STATUS Status; >> UINTN ResultSize; >> + UINTN StringSize; >> >> - ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StrSize (mSemihostFsLabel); >> + StringSize = StrSize (mSemihostFsLabel); >> + ResultSize = SIZE_OF_EFI_FILE_SYSTEM_INFO + StringSize; >> >> if (*BufferSize >= ResultSize) { >> ZeroMem (Buffer, ResultSize); >> @@ -843,7 +846,7 @@ GetFilesystemInfo ( >> Info->FreeSpace = 0; >> Info->BlockSize = 0; >> >> - StrCpy (Info->VolumeLabel, mSemihostFsLabel); >> + CopyMem (Info->VolumeLabel, mSemihostFsLabel, StringSize); >> } else { >> Status = EFI_BUFFER_TOO_SMALL; >> } >> @@ -903,7 +906,7 @@ FileGetInfo ( >> ResultSize = StrSize (mSemihostFsLabel); >> >> if (*BufferSize >= ResultSize) { >> - StrCpy (Buffer, mSemihostFsLabel); >> + CopyMem (Buffer, mSemihostFsLabel, *BufferSize); > > This is still wrong; here *BufferSize is the size of the recipient > buffer, passed in from the caller. As written, the code can overrun the > *source* buffer. Please use > > CopyMem (Buffer, mSemihostFsLabel, ResultSize); > > instead. > > With that update: > > Reviewed-by: Laszlo Ersek > I'll apply this update locally before I test the v2 series. > Thanks > Laszlo > >> Status = EFI_SUCCESS; >> } else { >> Status = EFI_BUFFER_TOO_SMALL; >> @@ -963,11 +966,12 @@ SetFileInfo ( >> return EFI_ACCESS_DENIED; >> } >> >> - AsciiFileName = AllocatePool (StrLen (Info->FileName) + 1); >> + Length = StrLen (Info->FileName) + 1; >> + AsciiFileName = AllocatePool (Length); >> if (AsciiFileName == NULL) { >> return EFI_OUT_OF_RESOURCES; >> } >> - UnicodeStrToAsciiStr (Info->FileName, AsciiFileName); >> + UnicodeStrToAsciiStrS (Info->FileName, AsciiFileName, Length); >> >> FileSizeIsDifferent = (Info->FileSize != Fcb->Info.FileSize); >> FileNameIsDifferent = (AsciiStrCmp (AsciiFileName, Fcb->FileName) != 0); >> >