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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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?

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