[Linaro-uefi] Platforms/StyxSpiFvDxe: fix potential boot crash on varstore write

Message ID 1489776322-6551-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 9197a349cc4b918e37edd76efb370aa79bd01ee0
Headers show

Commit Message

Ard Biesheuvel March 17, 2017, 6:45 p.m.
The varstore shadow FV is kept in sync with actual SPI flash read,
write and erase operations. Since we only expose a small slice of
the SPI flash for the variable store, we keep an internal LBA offset
and take it into account when translating shadow FV LBAs to actual
LBAs.

As it turns out, the erase routine applies the LBA offset incorrectly,
resulting in the wrong flash block being erased, and the wrong range
to be erased in the shadow FV, which could result in a crash if the
memory access is out of bounds.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Leif Lindholm March 18, 2017, 2:48 p.m. | #1
On Fri, Mar 17, 2017 at 06:45:22PM +0000, Ard Biesheuvel wrote:
> The varstore shadow FV is kept in sync with actual SPI flash read,
> write and erase operations. Since we only expose a small slice of
> the SPI flash for the variable store, we keep an internal LBA offset
> and take it into account when translating shadow FV LBAs to actual
> LBAs.
> 
> As it turns out, the erase routine applies the LBA offset incorrectly,
> resulting in the wrong flash block being erased, and the wrong range
> to be erased in the shadow FV, which could result in a crash if the
> memory access is out of bounds.

So, this is a very detailed and descriptive commmit message, but the
fix is just basically "don't apply offset twice"?

Anyway, this is obviously a fix:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c b/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c
> index 03fd9e816b96..f544af3eeb2d 100644
> --- a/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c
> +++ b/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c
> @@ -439,7 +439,6 @@ StyxSpiFvDxeErase (
>    for (Start = VA_ARG (Args, EFI_LBA);
>         Start != EFI_LBA_LIST_TERMINATOR;
>         Start = VA_ARG (Args, EFI_LBA)) {
> -    Start += mNvStorageLbaOffset;
>      Length = VA_ARG (Args, UINTN);
>      Status = mIscpDxeProtocol->AmdExecuteEraseFvBlockDxe (mIscpDxeProtocol,
>                                   (Start + mNvStorageLbaOffset) * BLOCK_SIZE,
> -- 
> 2.7.4
>
Ard Biesheuvel March 18, 2017, 2:49 p.m. | #2
On 18 March 2017 at 14:48, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Mar 17, 2017 at 06:45:22PM +0000, Ard Biesheuvel wrote:
>> The varstore shadow FV is kept in sync with actual SPI flash read,
>> write and erase operations. Since we only expose a small slice of
>> the SPI flash for the variable store, we keep an internal LBA offset
>> and take it into account when translating shadow FV LBAs to actual
>> LBAs.
>>
>> As it turns out, the erase routine applies the LBA offset incorrectly,
>> resulting in the wrong flash block being erased, and the wrong range
>> to be erased in the shadow FV, which could result in a crash if the
>> memory access is out of bounds.
>
> So, this is a very detailed and descriptive commmit message, but the
> fix is just basically "don't apply offset twice"?
>

Yes.

> Anyway, this is obviously a fix:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks

Patch

diff --git a/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c b/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c
index 03fd9e816b96..f544af3eeb2d 100644
--- a/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c
+++ b/Platforms/AMD/Styx/Drivers/StyxSpiFvDxe/StyxSpiFvDxe.c
@@ -439,7 +439,6 @@  StyxSpiFvDxeErase (
   for (Start = VA_ARG (Args, EFI_LBA);
        Start != EFI_LBA_LIST_TERMINATOR;
        Start = VA_ARG (Args, EFI_LBA)) {
-    Start += mNvStorageLbaOffset;
     Length = VA_ARG (Args, UINTN);
     Status = mIscpDxeProtocol->AmdExecuteEraseFvBlockDxe (mIscpDxeProtocol,
                                  (Start + mNvStorageLbaOffset) * BLOCK_SIZE,