From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web12.10252.1639396274373582470 for ; Mon, 13 Dec 2021 03:51:14 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=hB+o2j9k; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 0CC7FB80E0A for ; Mon, 13 Dec 2021 11:51:12 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C14EFC34602 for ; Mon, 13 Dec 2021 11:51:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1639396270; bh=r5bIW9oA0QSLiTLXW3M3ZiAC6CuYWZW75071BXZuQLQ=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=hB+o2j9kL5D2GwphPiXgRBJWw/A3stIN6kR3xQAMIAKdU8/ABoRgGnDXXBTiotxWZ go2Ha297UMmgjvHJHBvzcE2lRX8cTU19GVWbVFFlXEhN1xiOXYflUOpr0y5pubhFRf cQdNFZ7rSCPIDOC/RPozCtZ0y4ALaWkhlAWI1cl7wiUHGUzFfgSUynHkvoNEiF/Rt3 vnCRmhbsPBJ3Cvtedy+MFckBhEEZU+FNeevk5WTRkv4YB6peKctJx1VjBA3mObaxGl fgLUZn7cePMJxb+7jTNtA3WRvbM0d0Z8hz5RGnoJS/+al97ASWvTWrJj3HNmljfcnP CvoChs/mkMsQQ== Received: by mail-ot1-f54.google.com with SMTP id u18-20020a9d7212000000b00560cb1dc10bso17096808otj.11 for ; Mon, 13 Dec 2021 03:51:10 -0800 (PST) X-Gm-Message-State: AOAM533BXHFytllUlLbnzNE/U6Hwxv8qTIgK0XKdE/9hq/31DCHDuiC2 SCmm86c8wp6qbFauVBOS2uUAQjpp7tkln8MRfF0= X-Google-Smtp-Source: ABdhPJy3760JHJl18vqc0TuafQKj4rrWSp6JsOw+44VFJgv3z2NqDWYHTEoiczvQnhqY9eAXg2XD1WVOAIHemC8Ulsw= X-Received: by 2002:a9d:6c54:: with SMTP id g20mr24945979otq.30.1639396270057; Mon, 13 Dec 2021 03:51:10 -0800 (PST) MIME-Version: 1.0 References: <20211209065133.7748-1-wei6.xu@intel.com> In-Reply-To: <20211209065133.7748-1-wei6.xu@intel.com> From: "Ard Biesheuvel" Date: Mon, 13 Dec 2021 12:50:58 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel][Patch] StandaloneMmPkg/FvLib: Support large file with EFI_FFS_FILE_HEADER2. To: Wei6 Xu Cc: edk2-devel-groups-io , Ard Biesheuvel , Sami Mujawar , Jiewen Yao , Supreeth Venkatesh , Liming Gao Content-Type: text/plain; charset="UTF-8" On Thu, 9 Dec 2021 at 07:51, Wei6 Xu wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3769 > > Current FvLib will hit parse issue when encountering LARGE file, then > ignore latter ffs/section, thus causing required drivers not being > dispatched. Therefore, need to add support for EFI_FFS_FILE_HEADER2 > and EFI_COMMON_SECTION_HEADER2 in FvLib to fix this issue. > > Cc: Ard Biesheuvel > Cc: Sami Mujawar > Cc: Jiewen Yao > Cc: Supreeth Venkatesh > Cc: Liming Gao > Signed-off-by: Wei6 Xu > --- Is this an issue in practice? How large does the FV that is being dispatched into MM context have to be in order for this issue to trigger? > StandaloneMmPkg/Library/FvLib/FvLib.c | 65 ++++++++++++++++++++++++----------- > 1 file changed, 45 insertions(+), 20 deletions(-) > > diff --git a/StandaloneMmPkg/Library/FvLib/FvLib.c b/StandaloneMmPkg/Library/FvLib/FvLib.c > index aa36a35eff..89504b9ee9 100644 > --- a/StandaloneMmPkg/Library/FvLib/FvLib.c > +++ b/StandaloneMmPkg/Library/FvLib/FvLib.c > @@ -1,8 +1,8 @@ > /** @file > > -Copyright (c) 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.
> Copyright (c) 2016 - 2018, ARM Limited. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -61,22 +61,24 @@ CalculateHeaderChecksum ( > ) > { > UINT8 *ptr; > UINTN Index; > UINT8 Sum; > + UINTN Size; > > - Sum = 0; > - ptr = (UINT8 *)FileHeader; > + Sum = 0; > + ptr = (UINT8 *)FileHeader; > + Size = IS_FFS_FILE2 (FileHeader) ? sizeof (EFI_FFS_FILE_HEADER2) : sizeof (EFI_FFS_FILE_HEADER); > > - for (Index = 0; Index < sizeof (EFI_FFS_FILE_HEADER) - 3; Index += 4) { > + for (Index = 0; Index < Size - 3; Index += 4) { > Sum = (UINT8)(Sum + ptr[Index]); > Sum = (UINT8)(Sum + ptr[Index + 1]); > Sum = (UINT8)(Sum + ptr[Index + 2]); > Sum = (UINT8)(Sum + ptr[Index + 3]); > } > > - for ( ; Index < sizeof (EFI_FFS_FILE_HEADER); Index++) { > + for ( ; Index < Size; Index++) { > Sum = (UINT8)(Sum + ptr[Index]); > } > > // > // State field (since this indicates the different state of file). > @@ -155,11 +157,12 @@ FfsFindNextFile ( > } else { > // > // Length is 24 bits wide so mask upper 8 bits > // FileLength is adjusted to FileOccupiedSize as it is 8 byte aligned. > // > - FileLength = FFS_FILE_SIZE (*FileHeader); > + FileLength = IS_FFS_FILE2 (*FileHeader) ? > + FFS_FILE2_SIZE (*FileHeader) : FFS_FILE_SIZE (*FileHeader); > FileOccupiedSize = GET_OCCUPIED_SIZE (FileLength, 8); > FfsFileHeader = (EFI_FFS_FILE_HEADER *)((UINT8 *)*FileHeader + FileOccupiedSize); > } > > FileOffset = (UINT32)((UINT8 *)FfsFileHeader - (UINT8 *)FwVolHeader); > @@ -170,18 +173,25 @@ FfsFindNextFile ( > // > FileState = GetFileState (ErasePolarity, FfsFileHeader); > > switch (FileState) { > case EFI_FILE_HEADER_INVALID: > - FileOffset += sizeof (EFI_FFS_FILE_HEADER); > - FfsFileHeader = (EFI_FFS_FILE_HEADER *)((UINT8 *)FfsFileHeader + sizeof (EFI_FFS_FILE_HEADER)); > + if (IS_FFS_FILE2 (FfsFileHeader)) { > + FileOffset += sizeof (EFI_FFS_FILE_HEADER2); > + FfsFileHeader = (EFI_FFS_FILE_HEADER *)((UINT8 *)FfsFileHeader + sizeof (EFI_FFS_FILE_HEADER2)); > + } else { > + FileOffset += sizeof (EFI_FFS_FILE_HEADER); > + FfsFileHeader = (EFI_FFS_FILE_HEADER *)((UINT8 *)FfsFileHeader + sizeof (EFI_FFS_FILE_HEADER)); > + } > + > break; > > case EFI_FILE_DATA_VALID: > case EFI_FILE_MARKED_FOR_UPDATE: > if (CalculateHeaderChecksum (FfsFileHeader) == 0) { > - FileLength = FFS_FILE_SIZE (FfsFileHeader); > + FileLength = IS_FFS_FILE2 (FfsFileHeader) ? > + FFS_FILE2_SIZE (FfsFileHeader) : FFS_FILE_SIZE (FfsFileHeader); > FileOccupiedSize = GET_OCCUPIED_SIZE (FileLength, 8); > > if ((SearchType == FfsFileHeader->Type) || (SearchType == EFI_FV_FILETYPE_ALL)) { > *FileHeader = FfsFileHeader; > > @@ -195,11 +205,12 @@ FfsFindNextFile ( > } > > break; > > case EFI_FILE_DELETED: > - FileLength = FFS_FILE_SIZE (FfsFileHeader); > + FileLength = IS_FFS_FILE2 (FfsFileHeader) ? > + FFS_FILE2_SIZE (FfsFileHeader) : FFS_FILE_SIZE (FfsFileHeader); > FileOccupiedSize = GET_OCCUPIED_SIZE (FileLength, 8); > FileOffset += FileOccupiedSize; > FfsFileHeader = (EFI_FFS_FILE_HEADER *)((UINT8 *)FfsFileHeader + FileOccupiedSize); > break; > > @@ -251,11 +262,11 @@ FindFfsSectionInSections ( > > CurrentAddress = EndOfSection; > > Section = (EFI_COMMON_SECTION_HEADER *)(UINTN)CurrentAddress; > > - Size = SECTION_SIZE (Section); > + Size = IS_SECTION2 (Section) ? SECTION2_SIZE (Section) : SECTION_SIZE (Section); > if (Size < sizeof (*Section)) { > return EFI_VOLUME_CORRUPTED; > } > > EndOfSection = CurrentAddress + Size; > @@ -304,13 +315,17 @@ FfsFindSection ( > // > // Size is 24 bits wide so mask upper 8 bits. > // Does not include FfsFileHeader header size > // FileSize is adjusted to FileOccupiedSize as it is 8 byte aligned. > // > - Section = (EFI_COMMON_SECTION_HEADER *)(FfsFileHeader + 1); > - FileSize = FFS_FILE_SIZE (FfsFileHeader); > - FileSize -= sizeof (EFI_FFS_FILE_HEADER); > + if (IS_FFS_FILE2 (FfsFileHeader)) { > + Section = (EFI_COMMON_SECTION_HEADER *)((EFI_FFS_FILE_HEADER2 *)FfsFileHeader + 1); > + FileSize = FFS_FILE2_SIZE (FfsFileHeader) - sizeof (EFI_FFS_FILE_HEADER2); > + } else { > + Section = (EFI_COMMON_SECTION_HEADER *)(FfsFileHeader + 1); > + FileSize = FFS_FILE_SIZE (FfsFileHeader) - sizeof (EFI_FFS_FILE_HEADER); > + } > > Status = FindFfsSectionInSections ( > Section, > FileSize, > SectionType, > @@ -349,29 +364,39 @@ FfsFindSectionData ( > // > // Size is 24 bits wide so mask upper 8 bits. > // Does not include FfsFileHeader header size > // FileSize is adjusted to FileOccupiedSize as it is 8 byte aligned. > // > - Section = (EFI_COMMON_SECTION_HEADER *)(FfsFileHeader + 1); > - FileSize = FFS_FILE_SIZE (FfsFileHeader); > - FileSize -= sizeof (EFI_FFS_FILE_HEADER); > + if (IS_FFS_FILE2 (FfsFileHeader)) { > + Section = (EFI_COMMON_SECTION_HEADER *)((EFI_FFS_FILE_HEADER2 *)FfsFileHeader + 1); > + FileSize = FFS_FILE2_SIZE (FfsFileHeader) - sizeof (EFI_FFS_FILE_HEADER2); > + } else { > + Section = (EFI_COMMON_SECTION_HEADER *)(FfsFileHeader + 1); > + FileSize = FFS_FILE_SIZE (FfsFileHeader) - sizeof (EFI_FFS_FILE_HEADER); > + } > > *SectionData = NULL; > ParsedLength = 0; > while (ParsedLength < FileSize) { > if (Section->Type == SectionType) { > - *SectionData = (VOID *)(Section + 1); > - *SectionDataSize = SECTION_SIZE (Section); > + if (IS_SECTION2 (Section)) { > + *SectionData = (VOID *)((EFI_COMMON_SECTION_HEADER2 *)Section + 1); > + *SectionDataSize = SECTION2_SIZE (Section); > + } else { > + *SectionData = (VOID *)(Section + 1); > + *SectionDataSize = SECTION_SIZE (Section); > + } > + > return EFI_SUCCESS; > } > > // > // Size is 24 bits wide so mask upper 8 bits. > // SectionLength is adjusted it is 4 byte aligned. > // Go to the next section > // > - SectionLength = SECTION_SIZE (Section); > + SectionLength = IS_SECTION2 (Section) ? SECTION2_SIZE (Section) : SECTION_SIZE (Section); > SectionLength = GET_OCCUPIED_SIZE (SectionLength, 4); > > ParsedLength += SectionLength; > Section = (EFI_COMMON_SECTION_HEADER *)((UINT8 *)Section + SectionLength); > } > -- > 2.16.2.windows.1 >