From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::241]) (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 4C93D211D08FC for ; Wed, 13 Jun 2018 05:06:02 -0700 (PDT) Received: by mail-io0-x241.google.com with SMTP id f1-v6so3151722ioh.6 for ; Wed, 13 Jun 2018 05:06:02 -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=CVV00xE0yTmDS8ldaWB4a3yTXNpvt7SX3W9VMdGvSSQ=; b=JHThLiiAT0/XxgBnrm4zaiR1bVywcvcKqth18SPkufvAfDsSc4AwdDRsY28abs1oZo xY4GSn9J4CCc+NRMBEKe4tgJFLjIylgSEbo4aJaa7ikwoKNlPU/YSpw4SlB1i1Du5wn+ EhBZ0CgegypYWyilj7LlBmjlmPW9gPGsfc1zk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=CVV00xE0yTmDS8ldaWB4a3yTXNpvt7SX3W9VMdGvSSQ=; b=RPGwW3sIcHMWghQjDS7O0CtCxzsafg+B99RNNwvTEeJzwE4MLrKIVsO0ah+vV1sZ9w 1/URfpc0weK5p6ks2qwfuO+K9IjKcvVQ2byjGKAAV/LkK5tYdkS/9pFqxo0R1goS0bF9 jqK/7SD6Hxr469Z09FR8ghgcI+iXwtRPKbWN6xMznNQBjnI+PYuejSBIuE/ArLgznOPS Mx3G82GujgtLvpKtzRPL8vgbaUD2t06d53x0Zd0eOL4i9WXKmocATfZjtl+Xz6GWUTdH sI71Moqs0FkyQR1hy1Aav9QC25ifKL5GuOHFEzz27bcnCaSdlz+k5ZHfqZrjGAkEGJXF SghQ== X-Gm-Message-State: APt69E0csIXBWK1o5MjUaNpiChTiaMih+SQGfhYL1d43m6pMY4fs5vn4 jlELcAiGrYEUx8192sV6cHs++ageq8xT5Z9s1ToR6WFQy2M= X-Google-Smtp-Source: ADUXVKLPZ26IGGA+WUtooRRgwYAlZhm9PMh/g2tjl+i73QrbblPjcczgJk7k3JCubRQkvvV+SpVrOdg9hVYiwXCihzs= X-Received: by 2002:a6b:520d:: with SMTP id g13-v6mr4700587iob.60.1528891561412; Wed, 13 Jun 2018 05:06:01 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Wed, 13 Jun 2018 05:06:00 -0700 (PDT) In-Reply-To: <20180612230325.vkcjj4iuvpq2vbuk@bivouac.eciton.net> References: <20180607150818.14393-1-ard.biesheuvel@linaro.org> <20180607150818.14393-2-ard.biesheuvel@linaro.org> <20180612230325.vkcjj4iuvpq2vbuk@bivouac.eciton.net> From: Ard Biesheuvel Date: Wed, 13 Jun 2018 14:06:00 +0200 Message-ID: To: Leif Lindholm Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jun 2018 12:06:02 -0000 Content-Type: text/plain; charset="UTF-8" On 13 June 2018 at 01:03, Leif Lindholm wrote: > On Thu, Jun 07, 2018 at 05:08:17PM +0200, Ard Biesheuvel wrote: >> In commit 913fdda9f4b9 ("Silicon/SynQuacerPlatformFlashAccessLib: don't >> dereference FVB header fields"), we dropped all accesses to FVB header >> field, which was necessary because the flash partition may not in fact >> contain such a header. Instead, only an exact match on the base address >> of the FV compared to the base address of the capsule payload would >> result in a match, making it difficult to create capsules that only >> update a subset of the flash contents. >> >> Given that the FVB protocol provides a GetBlockSize() method that also >> returns the number of consecutive blocks of that size, and does not rely >> on the FVB header contents, we can actually infer the size of the flash >> partition, and use it to decide whether a capsule payload targets an >> area that is covered by this partition entirely. >> >> This optimization allows us to extend the FV description to include the >> SCP firmware partition without requiring us to actually provide a >> payload for that partition immediately, which is useful as a preparatory >> step. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Ard Biesheuvel >> --- >> Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c | 53 +++++++++----------- >> 1 file changed, 25 insertions(+), 28 deletions(-) >> >> diff --git a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c >> index ebb6ce189aa5..a6843c949a28 100644 >> --- a/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c >> +++ b/Silicon/Socionext/SynQuacer/Library/SynQuacerPlatformFlashAccessLib/SynQuacerPlatformFlashAccessLib.c >> @@ -44,8 +44,10 @@ STATIC >> EFI_STATUS >> GetFvbByAddress ( >> IN EFI_PHYSICAL_ADDRESS Address, >> + IN UINTN Length, >> OUT EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL **OutFvb, >> - OUT EFI_PHYSICAL_ADDRESS *FvbBaseAddress >> + OUT EFI_LBA *Lba, >> + OUT UINTN *BlockSize >> ) >> { >> EFI_STATUS Status; >> @@ -54,6 +56,8 @@ GetFvbByAddress ( >> UINTN Index; >> EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *Fvb; >> EFI_FVB_ATTRIBUTES_2 Attributes; >> + EFI_PHYSICAL_ADDRESS FvbBaseAddress; >> + UINTN NumberOfBlocks; >> >> // >> // Locate all handles with Firmware Volume Block protocol >> @@ -84,7 +88,7 @@ GetFvbByAddress ( >> // >> // Checks if the address range of this handle contains parameter Address >> // >> - Status = Fvb->GetPhysicalAddress (Fvb, FvbBaseAddress); >> + Status = Fvb->GetPhysicalAddress (Fvb, &FvbBaseAddress); >> if (EFI_ERROR (Status)) { >> continue; >> } >> @@ -102,8 +106,25 @@ GetFvbByAddress ( >> continue; >> } >> >> - if (Address == *FvbBaseAddress) { >> + Status = Fvb->GetBlockSize (Fvb, 0, BlockSize, &NumberOfBlocks); >> + if (EFI_ERROR (Status)) { >> + DEBUG ((DEBUG_INFO, "%a: failed to get FVB blocksize - %r, ignoring\n", >> + __FUNCTION__, Status)); >> + continue; >> + } >> + >> + if ((Length % *BlockSize) != 0) { >> + DEBUG ((DEBUG_INFO, >> + "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx, ignoring\n", >> + __FUNCTION__, Length, *BlockSize)); >> + Status = EFI_INVALID_PARAMETER; >> + continue; >> + } >> + >> + if (Address >= FvbBaseAddress && >> + (Address + Length) <= (FvbBaseAddress + *BlockSize * NumberOfBlocks)) { > > As I've already been giving Marcin a hard time about this today, could you add > parentheses around "Address >= FvbBaseAddress" and "*BlockSize * NumberOfBlocks"? > Sure. But to be consistent, shouldn't I also add parens around the entire term after the && then? I.e., + if ((Address >= FvbBaseAddress) && + ((Address + Length) <= (FvbBaseAddress + (*BlockSize * NumberOfBlocks)))) { I'm not sure why we like redundant parentheses so much in Tianocore, but I don't particularly mind either. >> *OutFvb = Fvb; >> + *Lba = (Address - FvbBaseAddress) / *BlockSize; >> Status = EFI_SUCCESS; >> break; >> } >> @@ -190,9 +211,7 @@ PerformFlashWriteWithProgress ( >> EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *Fvb; >> EFI_STATUS Status; >> UINTN BlockSize; >> - UINTN NumberOfBlocks; >> EFI_LBA Lba; >> - EFI_PHYSICAL_ADDRESS FvbBaseAddress; >> UINTN NumBytes; >> UINTN Remaining; >> >> @@ -216,7 +235,7 @@ PerformFlashWriteWithProgress ( >> // that covers the system firmware >> // >> Fvb = NULL; >> - Status = GetFvbByAddress (FlashAddress, &Fvb, &FvbBaseAddress); >> + Status = GetFvbByAddress (FlashAddress, Length, &Fvb, &Lba, &BlockSize); >> if (EFI_ERROR (Status)) { >> DEBUG ((DEBUG_ERROR, >> "%a: failed to locate FVB handle for address 0x%llx - %r\n", >> @@ -224,28 +243,6 @@ PerformFlashWriteWithProgress ( >> return Status; >> } >> >> - Status = Fvb->GetBlockSize(Fvb, 0, &BlockSize, &NumberOfBlocks); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, "%a: failed to get FVB blocksize - %r\n", >> - __FUNCTION__, Status)); >> - return Status; >> - } >> - >> - if ((Length % BlockSize) != 0) { >> - DEBUG ((DEBUG_ERROR, >> - "%a: Length 0x%lx is not a multiple of the blocksize 0x%lx\n", >> - __FUNCTION__, Length, BlockSize)); >> - return EFI_INVALID_PARAMETER; >> - } >> - >> - Lba = (FlashAddress - FvbBaseAddress) / BlockSize; >> - if (Lba > NumberOfBlocks - 1) { >> - DEBUG ((DEBUG_ERROR, >> - "%a: flash device with non-uniform blocksize not supported\n", >> - __FUNCTION__)); >> - return EFI_UNSUPPORTED; >> - } >> - >> // >> // Remap the region as device rather than uncached. >> // >> -- >> 2.17.0 >>