Re: [PATCH edk2-platforms 1/2] Silicon/SynQuacerPlatformFlashAccessLib: relax FV address check

[Leif Lindholm <leif.lindholm@linaro.org>]

On Wed, Jun 13, 2018 at 02:06:00PM +0200, Ard Biesheuvel wrote:
> On 13 June 2018 at 01:03, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org>
> >> ---
> >>  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?

Strictly speaking, yes.
The line break helps to demonstrate the separateness.

> 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.

Personally, it's because I neither have nor want the evaluation order
of C expressions hard-wired into my visual cortex.

I usually give * and / vs + and - the slip because I've failed to keep
those out.

/
    Leif

> >>        *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
> >>