From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::433; helo=mail-wr1-x433.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) (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 E9A3421180F35 for ; Mon, 29 Oct 2018 04:09:35 -0700 (PDT) Received: by mail-wr1-x433.google.com with SMTP id g9-v6so8199304wrq.4 for ; Mon, 29 Oct 2018 04:09:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=pbLBJM5yNchaDwSGTTOliN3J4AM3BcnO41HFwgoTv7I=; b=YVhtgeJafvoG3Ew0PqrE1i58+SAgy7i0Vil7qaDZXKlvIiX7N/gesP8VcAcq0iMCiN HsPYvzKIYN5QGU0BGG6I72wBydZkkNbnXIDCYTkEHSrz0uVacYCJ8mDtPFq0X33vAzrs V50rxi4D0inevbsxpsw6JGTv86w1ytZqPyA9c= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=pbLBJM5yNchaDwSGTTOliN3J4AM3BcnO41HFwgoTv7I=; b=qgKD7D6Rvds8VNLc375mO2ttMQZrs59jeleYEw3dg/q2oQrcLB6QIVQ4UiBlD65ves 4nKKyLlNebKfl5mKTUxkQeivYsyBz+S6aNI6ZrNBAheo8AENsf0+teMNBWj3G4Lg9BkU pN0FF04ZnySIJsgp/XEQhBErF4T58DcWrseGmKxkdb+j44DmC10/U6BL2daxlHkC7s1g V7+7+vShVUJO7hPyR7fEU2ymGTaRvtcfA3xlhd5+c6JHvrRQqUUYrwVSouIiFdzBILlS CjWl2AOo39joJqRX5QpWSESUwxzaZ8vpSJoNp4x5Sf+i6f057Zf1uycoG9ExRL+wg4dJ Dw7w== X-Gm-Message-State: AGRZ1gK6IPwVlL7akA/G5AOndySxvIAu6rcJfGpyDg67WLYtFFBTfBtL LI5qWxgdvgIkY3OB7cdpyzdoPw== X-Google-Smtp-Source: AJdET5fbSDwE3ye37wcv98MMsW21Ky+Vom1OCpDsY2020SgyRpHW7a6MeQrMjCknOP7Pxf1ybqYoKg== X-Received: by 2002:adf:c748:: with SMTP id b8-v6mr14271552wrh.134.1540811373777; Mon, 29 Oct 2018 04:09:33 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 78-v6sm10680659wma.38.2018.10.29.04.09.32 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 29 Oct 2018 04:09:32 -0700 (PDT) Date: Mon, 29 Oct 2018 11:09:30 +0000 From: Leif Lindholm To: Hao Wu Cc: edk2-devel@lists.01.org, Ruiyu Ni , Star Zeng , Paulo Alcantara Message-ID: <20181029110930.56pzdhdzhkn2ixcy@bivouac.eciton.net> References: <20181026075457.6280-1-hao.a.wu@intel.com> MIME-Version: 1.0 In-Reply-To: <20181026075457.6280-1-hao.a.wu@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] MdeModulePkg/UdfDxe: Additional checks for ResolveSymlink() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Oct 2018 11:09:36 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Hao Wu, I have no comment about the code changes, but would it be possible to split this up into three separate patches? They are three separate changes, but as it stands, it is not _obvious_ that they do not have interdependencies. Splitting them up would make that immediately obvious. You already have the three commit messages :) Best Regards, Leif On Fri, Oct 26, 2018 at 03:54:57PM +0800, Hao Wu wrote: > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1279 > > The commit will add 3 types of checks for function ResolveSymlink(): > > A. Check for the value of 'Component Type' field within a Path Component > > According to the ECMA-167 standard (3rd Edition - June 1997), Section > 14.16.1.1, valid values are 1 to 5. All other values will be treated as a > corrupted volume. > > B. Check for the content pointed by 'File' > > Since content within 'File' is the output data for ResolveSymlink(). > Checks is added to ensure the content in 'File' is valid. Otherwise, > possible null pointer dereference issue will occur during the subsequent > usage of the data returned by ResolveSymlink(). > > C. Check for possible memory double free/use after free case > > For codes: > > if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent, > sizeof (UDF_FILE_INFO)) != 0) { > CleanupFileInformation (&PreviousFile); > } > > CopyMem ((VOID *)&PreviousFile, (VOID *)File, sizeof (UDF_FILE_INFO)); > > If the contents in 'PreviousFile' and 'File' are the same, call to > "CleanupFileInformation (&PreviousFile);" will free the buffers in 'File' > as well. This will lead to potential memory double free/use after free > issues. > > Cc: Paulo Alcantara > Cc: Paulo Alcantara > Cc: Ruiyu Ni > Cc: Star Zeng > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Hao Wu > --- > .../Universal/Disk/UdfDxe/FileSystemOperations.c | 30 ++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c > index b9ebddfe62..a89e5ba9ff 100644 > --- a/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c > +++ b/MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c > @@ -2145,6 +2145,8 @@ ResolveSymlink ( > UINT8 CompressionId; > UDF_FILE_INFO PreviousFile; > > + ZeroMem ((VOID *)File, sizeof (UDF_FILE_INFO)); > + > // > // Symlink files on UDF volumes do not contain so much data other than > // Path Components which resolves to real filenames, so it's OK to read in > @@ -2257,6 +2259,13 @@ ResolveSymlink ( > } > FileName[Index] = L'\0'; > break; > + default: > + // > + // Accoring to the ECMA-167 standard (3rd Edition - June 1997), Section > + // 14.16.1.1, all other values are reserved. > + // > + Status = EFI_VOLUME_CORRUPTED; > + goto Error_Find_File; > } > > // > @@ -2281,8 +2290,18 @@ ResolveSymlink ( > break; > } > > - if (CompareMem ((VOID *)&PreviousFile, (VOID *)Parent, > - sizeof (UDF_FILE_INFO)) != 0) { > + // > + // Check the content in the file info pointed by File. > + // > + if ((File->FileEntry == NULL) || (File->FileIdentifierDesc == NULL)) { > + Status = EFI_VOLUME_CORRUPTED; > + goto Error_Find_File; > + } > + > + if ((CompareMem ((VOID *)&PreviousFile, (VOID *)Parent, > + sizeof (UDF_FILE_INFO)) != 0) && > + (CompareMem ((VOID *)&PreviousFile, (VOID *)File, > + sizeof (UDF_FILE_INFO)) != 0)) { > CleanupFileInformation (&PreviousFile); > } > > @@ -2294,6 +2313,13 @@ ResolveSymlink ( > // > FreePool (ReadFileInfo.FileData); > > + // > + // Check the content in the resolved file info. > + // > + if ((File->FileEntry == NULL) || (File->FileIdentifierDesc == NULL)) { > + return EFI_VOLUME_CORRUPTED; > + } > + > return EFI_SUCCESS; > > Error_Find_File: > -- > 2.12.0.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel