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

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

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"?

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