[edk2-devel] [PATCH 1/1] OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads

[edk2-devel] [PATCH 1/1] OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads

[Gerd Hoffmann]

In some cases (specifically when the flash update region is
small but crosses a multiple of P30_MAX_BUFFER_SIZE_IN_BYTES)
NorFlashWriteSingleBlock reads only one instead of two
P30_MAX_BUFFER_SIZE_IN_BYTES blocks into the shadow buffer.

That leads to random crap being written to the second block,
which in turn can corrupt both the variable store and the
FTW work space.

This patch fixes the calculation.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 1afd60ce66eb..cdc809d75e3d 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -566,7 +566,7 @@ NorFlashWriteSingleBlock (
                Instance,
                Lba,
                Offset & ~BOUNDARY_OF_32_WORDS,
-               (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
+               (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1,
                Instance->ShadowBuffer
                );
     if (EFI_ERROR (Status)) {
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113618): https://edk2.groups.io/g/devel/message/113618
Mute This Topic: https://groups.io/mt/103661868/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads

[Laszlo Ersek]

On 1/11/24 14:36, Gerd Hoffmann wrote:
> In some cases (specifically when the flash update region is
> small but crosses a multiple of P30_MAX_BUFFER_SIZE_IN_BYTES)
> NorFlashWriteSingleBlock reads only one instead of two
> P30_MAX_BUFFER_SIZE_IN_BYTES blocks into the shadow buffer.
> 
> That leads to random crap being written to the second block,
> which in turn can corrupt both the variable store and the
> FTW work space.
> 
> This patch fixes the calculation.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 1afd60ce66eb..cdc809d75e3d 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -566,7 +566,7 @@ NorFlashWriteSingleBlock (
>                 Instance,
>                 Lba,
>                 Offset & ~BOUNDARY_OF_32_WORDS,
> -               (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
> +               (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1,
>                 Instance->ShadowBuffer
>                 );
>      if (EFI_ERROR (Status)) {

This patch looks like the output of an excellent bug analysis. I'll need
more time to review this. If you have a ticket with the analysis
captured (actual numbers, debugging logs, a concrete backtrace / call
chain triggering the issue, etc), I'd appreciate a reference. (Perhaps
include some of the key items in the commit message too?)

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113705): https://edk2.groups.io/g/devel/message/113705
Mute This Topic: https://groups.io/mt/103661868/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH 1/1] OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads

[Gerd Hoffmann]

On Fri, Jan 12, 2024 at 10:47:43AM +0100, Laszlo Ersek wrote:
> On 1/11/24 14:36, Gerd Hoffmann wrote:
> > In some cases (specifically when the flash update region is
> > small but crosses a multiple of P30_MAX_BUFFER_SIZE_IN_BYTES)
> > NorFlashWriteSingleBlock reads only one instead of two
> > P30_MAX_BUFFER_SIZE_IN_BYTES blocks into the shadow buffer.
> > 
> > That leads to random crap being written to the second block,
> > which in turn can corrupt both the variable store and the
> > FTW work space.
> > 
> > This patch fixes the calculation.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> > index 1afd60ce66eb..cdc809d75e3d 100644
> > --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> > +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> > @@ -566,7 +566,7 @@ NorFlashWriteSingleBlock (
> >                 Instance,
> >                 Lba,
> >                 Offset & ~BOUNDARY_OF_32_WORDS,
> > -               (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
> > +               (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1,
> >                 Instance->ShadowBuffer
> >                 );
> >      if (EFI_ERROR (Status)) {
> 
> This patch looks like the output of an excellent bug analysis. I'll need
> more time to review this. If you have a ticket with the analysis
> captured (actual numbers, debugging logs, a concrete backtrace / call
> chain triggering the issue, etc), I'd appreciate a reference.

Yea, that was a few days browsing through the flash and Ftw code,
sprinkling in debug logging, learning how all this works.  Have a shell
script resetting a VM in a loop with slowly increasing delay, to trigger
the code paths recovering from interrupted flash writes.

Finding 0xaf (aka PcdDebugClearMemoryValue) in not-yet used places of
the ftw log (which should have been 0xff) was the bit of information
which finally made me find the root cause of the flash corruption.

I have some followup patches I'll send out as patch series in a moment,
one of them adds variables here to make the whole logic easier to
understand.

stay tuned & take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113711): https://edk2.groups.io/g/devel/message/113711
Mute This Topic: https://groups.io/mt/103661868/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-