[edk2-devel] [PATCH v2 1/1] OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateBounceBuffer

[edk2-devel] [PATCH v2 1/1] OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateBounceBuffer

[Gerd Hoffmann]

Searching for an unused bounce buffer in mReservedMemBitmap and
reserving the buffer by flipping the bit is a critical section
which must not be interrupted.  Raise the TPL level to ensure
that.

Without this fix it can happen that IoMmuDxe hands out the same
bounce buffer twice, causing trouble down the road.  Seen happening
in practice with VirtioNetDxe setting up the network interface (and
calling into IoMmuDxe from a polling timer callback) in parallel with
Boot Manager doing some disk I/O.  An ASSERT() in VirtioNet caught
the buffer inconsistency.

Full story with lots of details and discussions is available here:
https://bugzilla.redhat.com/show_bug.cgi?id=2211060

v2:
 - add locking to IoMmuFreeBounceBuffer too, clearing bits in
   mReservedMemBitmap is not guaranteed to be atomic (Michael Brown).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
index c8f6cf4818e8..103003cae376 100644
--- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
+++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
@@ -367,7 +367,9 @@ IoMmuAllocateBounceBuffer (
 {
   EFI_STATUS  Status;
   UINT32      ReservedMemBitmap;
+  EFI_TPL     OldTpl;
 
+  OldTpl            = gBS->RaiseTPL (TPL_NOTIFY);
   ReservedMemBitmap = 0;
   Status            = InternalAllocateBuffer (
                         Type,
@@ -378,6 +380,7 @@ IoMmuAllocateBounceBuffer (
                         );
   MapInfo->ReservedMemBitmap = ReservedMemBitmap;
   mReservedMemBitmap        |= ReservedMemBitmap;
+  gBS->RestoreTPL (OldTpl);
 
   ASSERT (Status == EFI_SUCCESS);
 
@@ -395,6 +398,8 @@ IoMmuFreeBounceBuffer (
   IN OUT     MAP_INFO  *MapInfo
   )
 {
+  EFI_TPL  OldTpl;
+
   if (MapInfo->ReservedMemBitmap == 0) {
     gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
   } else {
@@ -407,9 +412,11 @@ IoMmuFreeBounceBuffer (
       mReservedMemBitmap,
       mReservedMemBitmap & ((UINT32)(~MapInfo->ReservedMemBitmap))
       ));
+    OldTpl                     = gBS->RaiseTPL (TPL_NOTIFY);
     MapInfo->PlainTextAddress  = 0;
     mReservedMemBitmap        &= (UINT32)(~MapInfo->ReservedMemBitmap);
     MapInfo->ReservedMemBitmap = 0;
+    gBS->RestoreTPL (OldTpl);
   }
 
   return EFI_SUCCESS;
-- 
2.41.0



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

Re: [edk2-devel] [PATCH v2 1/1] OvmfPkg/IoMmuDxe: add locking to IoMmuAllocateBounceBuffer

[Ard Biesheuvel]

On Wed, 19 Jul 2023 at 18:31, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Searching for an unused bounce buffer in mReservedMemBitmap and
> reserving the buffer by flipping the bit is a critical section
> which must not be interrupted.  Raise the TPL level to ensure
> that.
>
> Without this fix it can happen that IoMmuDxe hands out the same
> bounce buffer twice, causing trouble down the road.  Seen happening
> in practice with VirtioNetDxe setting up the network interface (and
> calling into IoMmuDxe from a polling timer callback) in parallel with
> Boot Manager doing some disk I/O.  An ASSERT() in VirtioNet caught
> the buffer inconsistency.
>
> Full story with lots of details and discussions is available here:
> https://bugzilla.redhat.com/show_bug.cgi?id=2211060
>
> v2:
>  - add locking to IoMmuFreeBounceBuffer too, clearing bits in
>    mReservedMemBitmap is not guaranteed to be atomic (Michael Brown).
>

Please put this under the --- so I don't have to remove manually it
when applying.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Pushed as #4665

Thanks,

> ---
>  OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> index c8f6cf4818e8..103003cae376 100644
> --- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> +++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c
> @@ -367,7 +367,9 @@ IoMmuAllocateBounceBuffer (
>  {
>    EFI_STATUS  Status;
>    UINT32      ReservedMemBitmap;
> +  EFI_TPL     OldTpl;
>
> +  OldTpl            = gBS->RaiseTPL (TPL_NOTIFY);
>    ReservedMemBitmap = 0;
>    Status            = InternalAllocateBuffer (
>                          Type,
> @@ -378,6 +380,7 @@ IoMmuAllocateBounceBuffer (
>                          );
>    MapInfo->ReservedMemBitmap = ReservedMemBitmap;
>    mReservedMemBitmap        |= ReservedMemBitmap;
> +  gBS->RestoreTPL (OldTpl);
>
>    ASSERT (Status == EFI_SUCCESS);
>
> @@ -395,6 +398,8 @@ IoMmuFreeBounceBuffer (
>    IN OUT     MAP_INFO  *MapInfo
>    )
>  {
> +  EFI_TPL  OldTpl;
> +
>    if (MapInfo->ReservedMemBitmap == 0) {
>      gBS->FreePages (MapInfo->PlainTextAddress, MapInfo->NumberOfPages);
>    } else {
> @@ -407,9 +412,11 @@ IoMmuFreeBounceBuffer (
>        mReservedMemBitmap,
>        mReservedMemBitmap & ((UINT32)(~MapInfo->ReservedMemBitmap))
>        ));
> +    OldTpl                     = gBS->RaiseTPL (TPL_NOTIFY);
>      MapInfo->PlainTextAddress  = 0;
>      mReservedMemBitmap        &= (UINT32)(~MapInfo->ReservedMemBitmap);
>      MapInfo->ReservedMemBitmap = 0;
> +    gBS->RestoreTPL (OldTpl);
>    }
>
>    return EFI_SUCCESS;
> --
> 2.41.0
>
>
>
> 
>
>


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