[PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
(inherited from memory_region_find()), but both SEV callers were failing
to release it.

Fixes: c7f7e6970d3b ("sev: add sev-inject-launch-secret")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/system/memory.h | 2 ++
 target/i386/sev.c       | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/system/memory.h b/include/system/memory.h
index 1417132f6d9..53f65a224bb 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -1343,6 +1343,8 @@ void memory_region_ref(MemoryRegion *mr);
  */
 void memory_region_unref(MemoryRegion *mr);
 
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(MemoryRegion, memory_region_unref)
+
 /**
  * memory_region_init_io: Initialize an I/O memory region.
  *
diff --git a/target/i386/sev.c b/target/i386/sev.c
index b44b5a1c2b9..3ae922779d7 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1573,7 +1573,7 @@ snp_populate_metadata_pages(SevSnpGuestState *sev_snp,
     OvmfSevMetadataDesc *desc;
     int type, ret, i;
     void *hva;
-    MemoryRegion *mr = NULL;
+    g_autoptr(MemoryRegion) mr = NULL;
 
     for (i = 0; i < metadata->num_desc; i++) {
         desc = &metadata->descs[i];
@@ -2067,7 +2067,7 @@ int sev_inject_launch_secret(const char *packet_hdr, const char *secret,
     int error, ret = 1;
     void *hva;
     gsize hdr_sz = 0, data_sz = 0;
-    MemoryRegion *mr = NULL;
+    g_autoptr(MemoryRegion) mr = NULL;
     SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
 
     if (!sev_common) {
-- 
2.54.0

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Peter Xu]

On Thu, May 21, 2026 at 06:54:51PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
> (inherited from memory_region_find()), but both SEV callers were failing
> to release it.
> 
> Fixes: c7f7e6970d3b ("sev: add sev-inject-launch-secret")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Marc-André Lureau]

Hi

On Thu, May 21, 2026 at 8:25 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Thu, May 21, 2026 at 06:54:51PM +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
> > (inherited from memory_region_find()), but both SEV callers were failing
> > to release it.
> >
> > Fixes: c7f7e6970d3b ("sev: add sev-inject-launch-secret")
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>

If nobody object, I'll queue this patch before it is forgotten.

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Zhao Liu]

On Thu, May 21, 2026 at 06:54:51PM +0400, marcandre.lureau@redhat.com wrote:
> Date: Thu, 21 May 2026 18:54:51 +0400
> From: marcandre.lureau@redhat.com
> Subject: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in
>  gpa2hva callers
> 
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
> (inherited from memory_region_find()), but both SEV callers were failing
> to release it.
> 
> Fixes: c7f7e6970d3b ("sev: add sev-inject-launch-secret")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/system/memory.h | 2 ++
>  target/i386/sev.c       | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Philippe Mathieu-Daudé]

Hi,

On 21/5/26 16:54, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
> (inherited from memory_region_find()), but both SEV callers were failing
> to release it.

Comment orthogonal to your fix; odd to have a non-monitor code
call an undocumented monitor method. Should the method be moved
out of monitor/hmp-cmds.c somewhere into system/ and documented
(likely in system/physmem.c, also moving the declaration to
"system/memory.h")? Besides some day we should be able to build
without monitor.

> Fixes: c7f7e6970d3b ("sev: add sev-inject-launch-secret")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   include/system/memory.h | 2 ++
>   target/i386/sev.c       | 4 ++--
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/system/memory.h b/include/system/memory.h
> index 1417132f6d9..53f65a224bb 100644
> --- a/include/system/memory.h
> +++ b/include/system/memory.h
> @@ -1343,6 +1343,8 @@ void memory_region_ref(MemoryRegion *mr);
>    */
>   void memory_region_unref(MemoryRegion *mr);
>   
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(MemoryRegion, memory_region_unref)
> +
>   /**
>    * memory_region_init_io: Initialize an I/O memory region.
>    *

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:

> Hi,
>
> On 21/5/26 16:54, marcandre.lureau@redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
>> (inherited from memory_region_find()), but both SEV callers were failing
>> to release it.
>
> Comment orthogonal to your fix; odd to have a non-monitor code
> call an undocumented monitor method.

Which one?

>                                      Should the method be moved
> out of monitor/hmp-cmds.c somewhere into system/ and documented
> (likely in system/physmem.c, also moving the declaration to
> "system/memory.h")? Besides some day we should be able to build
> without monitor.
>
>> Fixes: c7f7e6970d3b ("sev: add sev-inject-launch-secret")
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Philippe Mathieu-Daudé]

On 23/7/26 11:45, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:
> 
>> Hi,
>>
>> On 21/5/26 16:54, marcandre.lureau@redhat.com wrote:
>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
>>> (inherited from memory_region_find()), but both SEV callers were failing
>>> to release it.
>>
>> Comment orthogonal to your fix; odd to have a non-monitor code
>> call an undocumented monitor method.
> 
> Which one?

$ git grep -w gpa2hva
hmp-commands.hx:538:        .name       = "gpa2hva",
hmp-commands.hx:546:``gpa2hva`` *addr*
include/monitor/monitor.h:57:void *gpa2hva(MemoryRegion **p_mr, hwaddr 
addr, uint64_t size, Error **errp);
monitor/hmp-cmds.c:715:void *gpa2hva(MemoryRegion **p_mr, hwaddr addr, 
uint64_t size, Error **errp)
monitor/hmp-cmds.c:753:    ptr = gpa2hva(&mr, addr, 1, &local_err);
monitor/hmp-cmds.c:827:    ptr = gpa2hva(&mr, addr, 1, &local_err);
target/i386/sev.c:1579:        hva = gpa2hva(&mr, desc->base, desc->len, 
NULL);
target/i386/sev.c:2093:    hva = gpa2hva(&mr, gpa, data_sz, errp);
$

> 
>>                                       Should the method be moved
>> out of monitor/hmp-cmds.c somewhere into system/ and documented
>> (likely in system/physmem.c, also moving the declaration to
>> "system/memory.h")? Besides some day we should be able to build
>> without monitor.
>>
>>> Fixes: c7f7e6970d3b ("sev: add sev-inject-launch-secret")
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> 

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Peter Maydell]

On Thu, 23 Jul 2026 at 10:46, Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:
>
> > Hi,
> >
> > On 21/5/26 16:54, marcandre.lureau@redhat.com wrote:
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
> >> (inherited from memory_region_find()), but both SEV callers were failing
> >> to release it.
> >
> > Comment orthogonal to your fix; odd to have a non-monitor code
> > call an undocumented monitor method.
>
> Which one?

The gpa2hva() function, presumably. It does seem a bit odd to
have the target/i386 code directly calling this. The function
in monitor.c works on the system memory AddressSpace and
assumes that there is only one kind of "guest physical address",
because that's the implicit "everything is an old i386 PC"
kind of assumption that got backed into the human monitor
command. But it's not really something we should be doing
more widely.

The i386/sev code looks rather odd to me. It seems to be
using gpa2hva as a way to get at the host memory that is
backing the guest memory. Why can't it use the usual
address_space_map() etc APIs for this? Worse, it looks like
it keeps that host pointer around in a SevLaunchUpdateData
struct, and it's not immediately obvious to me that it won't
go wrong if the memory map later gets moved around and that
pointer becomes invalid. And it looks like it writes into
guest memory in a way that will bypass the dirty-tracking.

thanks
-- PMM

Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in gpa2hva callers

[Zhao Liu]

On Thu, Jul 23, 2026 at 11:45:47AM +0100, Peter Maydell wrote:
> Date: Thu, 23 Jul 2026 11:45:47 +0100
> From: Peter Maydell <peter.maydell@linaro.org>
> Subject: Re: [PATCH] target/i386/sev: fix MemoryRegion reference leaks in
>  gpa2hva callers
> 
> On Thu, 23 Jul 2026 at 10:46, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:
> >
> > > Hi,
> > >
> > > On 21/5/26 16:54, marcandre.lureau@redhat.com wrote:
> > >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >> gpa2hva() returns a referenced MemoryRegion via its p_mr out-parameter
> > >> (inherited from memory_region_find()), but both SEV callers were failing
> > >> to release it.
> > >
> > > Comment orthogonal to your fix; odd to have a non-monitor code
> > > call an undocumented monitor method.
> >
> > Which one?
> 
> The gpa2hva() function, presumably. It does seem a bit odd to
> have the target/i386 code directly calling this. The function
> in monitor.c works on the system memory AddressSpace and
> assumes that there is only one kind of "guest physical address",
> because that's the implicit "everything is an old i386 PC"
> kind of assumption that got backed into the human monitor
> command. But it's not really something we should be doing
> more widely.
>
> The i386/sev code looks rather odd to me. It seems to be
> using gpa2hva as a way to get at the host memory that is
> backing the guest memory. Why can't it use the usual
> address_space_map() etc APIs for this?

Yes, I agree.

For sev_inject_launch_secret, it's straightforward to use
address_space_map().

> Worse, it looks like
> it keeps that host pointer around in a SevLaunchUpdateData
> struct, and it's not immediately obvious to me that it won't
> go wrong if the memory map later gets moved around and that
> pointer becomes invalid. And it looks like it writes into
> guest memory in a way that will bypass the dirty-tracking.

For snp_populate_metadata_pages() and SevLaunchUpdateData,
I think maybe we can use address_space_map() to get pointer,
and add a flag to SevLaunchUpdateData marking that data entry
owns a mapping. Then sev_snp_launch_finish() can unmap if that
flag is set, in its loop.

Thanks,
Zhao