[PATCH 0/3] migration: A few fixes on coverity reports

[PATCH 0/3] migration: A few fixes on coverity reports

[Peter Xu]

A small series that fixes three coverity reported issues.  Please review,
thanks.

Peter Xu (3):
  migration: Fix error leak in postcopy_ram_listen_thread()
  migration/cpr: Fix coverity report in cpr_exec_persist_state()
  migration/cpr: Fix UAF in cpr_exec_cb() when execvp() fails

 migration/cpr-exec.c | 9 ++++++---
 migration/savevm.c   | 7 ++++---
 2 files changed, 10 insertions(+), 6 deletions(-)

-- 
2.50.1

[PATCH 1/3] migration: Fix error leak in postcopy_ram_listen_thread()

[Peter Xu]

As reported and analyzed by Peter:

https://lore.kernel.org/r/CAFEAcA9otBWtR7rPQ0Y9aBm+7ZWJzd4VWpXrAmGr8XspPn+zpw@mail.gmail.com

Fix it by freeing the error.  When at it, always reset the local_err
pointer in both paths.

Cc: Arun Menon <armenon@redhat.com>
Resolves: Coverity CID 1641390
Fixes: 94272d9b45 ("migration: Capture error in postcopy_ram_listen_thread()")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/savevm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index aafa40d779..635fa2f918 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2136,17 +2136,18 @@ static void *postcopy_ram_listen_thread(void *opaque)
         if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING &&
             !migrate_postcopy_ram() && migrate_dirty_bitmaps())
         {
-            error_report("%s: loadvm failed during postcopy: %d. All states "
+            error_report("%s: loadvm failed during postcopy: %s. All states "
                          "are migrated except dirty bitmaps. Some dirty "
                          "bitmaps may be lost, and present migrated dirty "
                          "bitmaps are correctly migrated and valid.",
-                         __func__, load_res);
+                         __func__, error_get_pretty(local_err));
+            g_clear_pointer(&local_err, error_free);
             load_res = 0; /* prevent further exit() */
         } else {
             error_prepend(&local_err,
                           "loadvm failed during postcopy: %d: ", load_res);
             migrate_set_error(migr, local_err);
-            error_report_err(local_err);
+            g_clear_pointer(&local_err, error_report_err);
             migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
                                            MIGRATION_STATUS_FAILED);
         }
-- 
2.50.1

[PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()

[Peter Xu]

Per reported and analyzed by Peter:

https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com

mfd leak is a false positive, try to use a coverity annotation (which I
didn't find manual myself, but still give it a shot).

Fix the other one by dumping an error message if setenv() failed.

Resolves: Coverity CID 1641391
Resolves: Coverity CID 1641392
Fixes: efc6587313 ("migration: cpr-exec save and load")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/cpr-exec.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index d57714bc5d..3cf44634a9 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
 void cpr_exec_persist_state(QEMUFile *f)
 {
     QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
+    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
     int mfd = dup(fioc->fd);
     char val[16];
 
     /* Remember mfd in environment for post-exec load */
     qemu_clear_cloexec(mfd);
     snprintf(val, sizeof(val), "%d", mfd);
-    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
+    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
+        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
+    }
 }
 
 static int cpr_exec_find_state(void)
-- 
2.50.1

[PATCH 3/3] migration/cpr: Fix UAF in cpr_exec_cb() when execvp() fails

[Peter Xu]

Per reported and analyzed by Peter:

https://lore.kernel.org/r/CAFEAcA82ih8RVCm-u1oxiS0V2K4rV4jMzNb13pAV=e2ivmiDRA@mail.gmail.com

Fix the issue by moving the error_setg_errno() earlier.  When at it, clear
argv variable after freed.

Resolves: Coverity CID 1641397
Fixes: a3eae205c6 ("migration: cpr-exec mode")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/cpr-exec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
index 3cf44634a9..15c9895a11 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -149,10 +149,10 @@ static void cpr_exec_cb(void *opaque)
      * exec should only fail if argv[0] is bogus, or has a permissions problem,
      * or the system is very short on resources.
      */
-    g_strfreev(argv);
+    error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
+    g_clear_pointer(&argv, g_strfreev);
     cpr_exec_unpreserve_fds();
 
-    error_setg_errno(&err, errno, "execvp %s failed", argv[0]);
     error_report_err(error_copy(err));
     migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
     migrate_set_error(s, err);
-- 
2.50.1

Re: [PATCH 1/3] migration: Fix error leak in postcopy_ram_listen_thread()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> As reported and analyzed by Peter:
>
> https://lore.kernel.org/r/CAFEAcA9otBWtR7rPQ0Y9aBm+7ZWJzd4VWpXrAmGr8XspPn+zpw@mail.gmail.com
>
> Fix it by freeing the error.  When at it, always reset the local_err
> pointer in both paths.
>
> Cc: Arun Menon <armenon@redhat.com>
> Resolves: Coverity CID 1641390
> Fixes: 94272d9b45 ("migration: Capture error in postcopy_ram_listen_thread()")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/savevm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index aafa40d779..635fa2f918 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2136,17 +2136,18 @@ static void *postcopy_ram_listen_thread(void *opaque)
>          if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING &&
>              !migrate_postcopy_ram() && migrate_dirty_bitmaps())
>          {
> -            error_report("%s: loadvm failed during postcopy: %d. All states "
> +            error_report("%s: loadvm failed during postcopy: %s. All states "

Do we want to keep the %d for consistency with the way we report the
error below?

loadvm failed during postcopy: %d: %s

>                           "are migrated except dirty bitmaps. Some dirty "
>                           "bitmaps may be lost, and present migrated dirty "
>                           "bitmaps are correctly migrated and valid.",
> -                         __func__, load_res);
> +                         __func__, error_get_pretty(local_err));
> +            g_clear_pointer(&local_err, error_free);
>              load_res = 0; /* prevent further exit() */
>          } else {
>              error_prepend(&local_err,
>                            "loadvm failed during postcopy: %d: ", load_res);
>              migrate_set_error(migr, local_err);
> -            error_report_err(local_err);
> +            g_clear_pointer(&local_err, error_report_err);
>              migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
>                                             MIGRATION_STATUS_FAILED);
>          }

Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Per reported and analyzed by Peter:
>
> https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com
>
> mfd leak is a false positive, try to use a coverity annotation (which I
> didn't find manual myself, but still give it a shot).
>
> Fix the other one by dumping an error message if setenv() failed.
>
> Resolves: Coverity CID 1641391
> Resolves: Coverity CID 1641392
> Fixes: efc6587313 ("migration: cpr-exec save and load")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/cpr-exec.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> index d57714bc5d..3cf44634a9 100644
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
> @@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
>  void cpr_exec_persist_state(QEMUFile *f)
>  {
>      QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
> +    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
>      int mfd = dup(fioc->fd);
>      char val[16];
>  
>      /* Remember mfd in environment for post-exec load */
>      qemu_clear_cloexec(mfd);
>      snprintf(val, sizeof(val), "%d", mfd);
> -    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
> +    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
> +        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
> +    }

Best to abort no? We don't want the rest of the code reading whatever
may be at that env variable and running with it.

>  }
>  
>  static int cpr_exec_find_state(void)

Re: [PATCH 3/3] migration/cpr: Fix UAF in cpr_exec_cb() when execvp() fails

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> Per reported and analyzed by Peter:
>
> https://lore.kernel.org/r/CAFEAcA82ih8RVCm-u1oxiS0V2K4rV4jMzNb13pAV=e2ivmiDRA@mail.gmail.com
>
> Fix the issue by moving the error_setg_errno() earlier.  When at it, clear
> argv variable after freed.
>
> Resolves: Coverity CID 1641397
> Fixes: a3eae205c6 ("migration: cpr-exec mode")
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH 1/3] migration: Fix error leak in postcopy_ram_listen_thread()

[Peter Xu]

On Tue, Oct 21, 2025 at 05:45:12PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > As reported and analyzed by Peter:
> >
> > https://lore.kernel.org/r/CAFEAcA9otBWtR7rPQ0Y9aBm+7ZWJzd4VWpXrAmGr8XspPn+zpw@mail.gmail.com
> >
> > Fix it by freeing the error.  When at it, always reset the local_err
> > pointer in both paths.
> >
> > Cc: Arun Menon <armenon@redhat.com>
> > Resolves: Coverity CID 1641390
> > Fixes: 94272d9b45 ("migration: Capture error in postcopy_ram_listen_thread()")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/savevm.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index aafa40d779..635fa2f918 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2136,17 +2136,18 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >          if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING &&
> >              !migrate_postcopy_ram() && migrate_dirty_bitmaps())
> >          {
> > -            error_report("%s: loadvm failed during postcopy: %d. All states "
> > +            error_report("%s: loadvm failed during postcopy: %s. All states "
> 
> Do we want to keep the %d for consistency with the way we report the
> error below?
> 
> loadvm failed during postcopy: %d: %s

I thought the string is verbose enough in an error report, but sure, I can
do that.

> 
> >                           "are migrated except dirty bitmaps. Some dirty "
> >                           "bitmaps may be lost, and present migrated dirty "
> >                           "bitmaps are correctly migrated and valid.",
> > -                         __func__, load_res);
> > +                         __func__, error_get_pretty(local_err));
> > +            g_clear_pointer(&local_err, error_free);
> >              load_res = 0; /* prevent further exit() */
> >          } else {
> >              error_prepend(&local_err,
> >                            "loadvm failed during postcopy: %d: ", load_res);
> >              migrate_set_error(migr, local_err);
> > -            error_report_err(local_err);
> > +            g_clear_pointer(&local_err, error_report_err);
> >              migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> >                                             MIGRATION_STATUS_FAILED);
> >          }
> 

-- 
Peter Xu

Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()

[Peter Xu]

On Tue, Oct 21, 2025 at 05:49:07PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Per reported and analyzed by Peter:
> >
> > https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com
> >
> > mfd leak is a false positive, try to use a coverity annotation (which I
> > didn't find manual myself, but still give it a shot).
> >
> > Fix the other one by dumping an error message if setenv() failed.
> >
> > Resolves: Coverity CID 1641391
> > Resolves: Coverity CID 1641392
> > Fixes: efc6587313 ("migration: cpr-exec save and load")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/cpr-exec.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> > index d57714bc5d..3cf44634a9 100644
> > --- a/migration/cpr-exec.c
> > +++ b/migration/cpr-exec.c
> > @@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
> >  void cpr_exec_persist_state(QEMUFile *f)
> >  {
> >      QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
> > +    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
> >      int mfd = dup(fioc->fd);
> >      char val[16];
> >  
> >      /* Remember mfd in environment for post-exec load */
> >      qemu_clear_cloexec(mfd);
> >      snprintf(val, sizeof(val), "%d", mfd);
> > -    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
> > +    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
> > +        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
> > +    }
> 
> Best to abort no? We don't want the rest of the code reading whatever
> may be at that env variable and running with it.

I didn't want to abort, because it's the same as QMP migrate failure at the
beginning.

If we want to do better, we can allow this function to return a failure
instead.

And.. when I was trying cpr-exec with no argv it already can crash QEMU.  I
think that's also a bug.

Let me prepare something better than this..  I'll likely add a new patch to
fix that too.

> 
> >  }
> >  
> >  static int cpr_exec_find_state(void)
> 

-- 
Peter Xu

Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Oct 21, 2025 at 05:49:07PM -0300, Fabiano Rosas wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > Per reported and analyzed by Peter:
>> >
>> > https://lore.kernel.org/r/CAFEAcA_mUQ2NeoguR5efrhw7XYGofnriWEA=+Dg+Ocvyam1wAw@mail.gmail.com
>> >
>> > mfd leak is a false positive, try to use a coverity annotation (which I
>> > didn't find manual myself, but still give it a shot).
>> >
>> > Fix the other one by dumping an error message if setenv() failed.
>> >
>> > Resolves: Coverity CID 1641391
>> > Resolves: Coverity CID 1641392
>> > Fixes: efc6587313 ("migration: cpr-exec save and load")
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/cpr-exec.c | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
>> > index d57714bc5d..3cf44634a9 100644
>> > --- a/migration/cpr-exec.c
>> > +++ b/migration/cpr-exec.c
>> > @@ -43,13 +43,16 @@ static QEMUFile *qemu_file_new_fd_output(int fd, const char *name)
>> >  void cpr_exec_persist_state(QEMUFile *f)
>> >  {
>> >      QIOChannelFile *fioc = QIO_CHANNEL_FILE(qemu_file_get_ioc(f));
>> > +    /* coverity[leaked_storage] - mfd intentionally kept open across exec() */
>> >      int mfd = dup(fioc->fd);
>> >      char val[16];
>> >  
>> >      /* Remember mfd in environment for post-exec load */
>> >      qemu_clear_cloexec(mfd);
>> >      snprintf(val, sizeof(val), "%d", mfd);
>> > -    g_setenv(CPR_EXEC_STATE_NAME, val, 1);
>> > +    if (!g_setenv(CPR_EXEC_STATE_NAME, val, 1)) {
>> > +        error_report("Setting env %s = %s failed", CPR_EXEC_STATE_NAME, val);
>> > +    }
>> 
>> Best to abort no? We don't want the rest of the code reading whatever
>> may be at that env variable and running with it.
>
> I didn't want to abort, because it's the same as QMP migrate failure at the
> beginning.
>
> If we want to do better, we can allow this function to return a failure
> instead.
>
> And.. when I was trying cpr-exec with no argv it already can crash QEMU.  I
> think that's also a bug.
>
> Let me prepare something better than this..  I'll likely add a new patch to
> fix that too.
>

Remember to copy Oracle folks.

>> 
>> >  }
>> >  
>> >  static int cpr_exec_find_state(void)
>> 

Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()

[Peter Xu]

On Wed, Oct 22, 2025 at 09:24:25AM -0300, Fabiano Rosas wrote:
> Remember to copy Oracle folks.

Do you know who's taking this over from Steve?  If we know, we should
update the MAINTAINERS file.

-- 
Peter Xu

Re: [PATCH 2/3] migration/cpr: Fix coverity report in cpr_exec_persist_state()

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Oct 22, 2025 at 09:24:25AM -0300, Fabiano Rosas wrote:
>> Remember to copy Oracle folks.
>
> Do you know who's taking this over from Steve?  If we know, we should
> update the MAINTAINERS file.

I'm not sure about maintainership but I'd be already glad to get testing
and reviews. Steve mentioned Mark Kanda on his email. I'm CCing Mark
here and he can tell us about it. =) No pressure, just making sure CPR
work is not falling between the cracks really.