| Message ID | 1489776322-6551-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 9197a349cc4b918e37edd76efb370aa79bd01ee0 |
| Headers | show |
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 >
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
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,
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(-)