Re: [PATCH] system: fix buffer overflow in tdx_parse_panic_message

["mlevitsk@redhat.com" <mlevitsk@redhat.com>]

On Wed, 2026-09-23 at 16:36 +0200, Paolo Bonzini wrote:
> On 9/23/26 02:45, Maxim Levitsky wrote:
> > tdx_parse_panic_message has code that detects if the guest supplied
> > message contains some unprintable characters and in this case prints a hex dump.
> > 
> > The code has a bug: it allocates 3 bytes for 'sprintf("%02x ")',
> > but sprintf in this case actually writes 4 bytes, because it adds the NULL
> > terminator.
> > 
> > To fix this, just use qemu_hexdump_line instead.
> > This patch was only compile tested.
> > 
> > Reported-by: Jordy Zomer <jordyzomer@google.com>
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3905
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >   system/runstate.c | 35 +++++++----------------------------
> >   1 file changed, 7 insertions(+), 28 deletions(-)
> > 
> > diff --git a/system/runstate.c b/system/runstate.c
> > index d3e64d2b6258..ebf09b192b4e 100644
> > --- a/system/runstate.c
> > +++ b/system/runstate.c
> > @@ -50,6 +50,7 @@
> >   #include "qemu/sockets.h"
> >   #include "qemu/timer.h"
> >   #include "qemu/thread.h"
> > +#include "qemu/cutils.h"
> >   #include "qom/object.h"
> >   #include "qom/object_interfaces.h"
> >   #include "system/cpu-timers.h"
> > @@ -760,10 +761,9 @@ static void qemu_system_wakeup(void)
> >       }
> >   }
> >   
> > -static char *tdx_parse_panic_message(char *message)
> > +static GString *tdx_parse_panic_message(char *message)
> >   {
> >       bool printable = false;
> > -    char *buf = NULL;
> >       int len = 0, i;
> >   
> >       /*
> > @@ -784,32 +784,11 @@ static char *tdx_parse_panic_message(char *message)
> >           }
> >       }
> >   
> > -    if (len == 0) {
> > -        buf = g_malloc(1);
> > -        buf[0] = '\0';
> > +    if (printable) {
> > +        return g_string_new(message);
> >       } else {
> > -        if (!printable) {
> > -            /* 3 = length of "%02x " */
> > -            buf = g_malloc(len * 3);
> > -            for (i = 0; i < len; i++) {
> > -                if (message[i] == '\0') {
> > -                    break;
> > -                } else {
> > -                    sprintf(buf + 3 * i, "%02x ", message[i]);
> > -                }
> > -            }
> > -            if (i > 0) {
> > -                /* replace the last ' '(space) to NULL */
> > -                buf[i * 3 - 1] = '\0';
> > -            } else {
> > -                buf[0] = '\0';
> > -            }
> > -        } else {
> > -            buf = g_strdup(message);
> > -        }
> > +        return qemu_hexdump_line(NULL, message, len, 1, 0);
> >       }
> > -
> > -    return buf;
> >   }
> >   
> >   void qemu_system_guest_panicked(GuestPanicInformation *info)
> > @@ -854,11 +833,11 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
> >                             info->u.s390.psw_mask,
> >                             info->u.s390.psw_addr);
> >           } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_TDX) {
> > -            char *message = tdx_parse_panic_message(info->u.tdx.message);
> > +            GString *message = tdx_parse_panic_message(info->u.tdx.message);
> >               qemu_log_mask(LOG_GUEST_ERROR,
> >                             "\nTDX guest reports fatal error."
> >                             " error code: 0x%" PRIx32 " error message:\"%s\"\n",
> > -                          info->u.tdx.error_code, message);
> > +                          info->u.tdx.error_code, message->str);
> >               g_free(message);
> 
> Freeing the GString* leaks message->str.
> 
> The easiest way to do it, which I have squashed on top of your patch, is
> 
>     g_autoptr(GString) message = ...
> 
> and dropping the g_free(message) line.  See for example sdbus_write_dump().

Oops, thanks for fixing this, I thought about that g_free(), but I guess not enough.

Best regards,
	Maxim Levitsky

> 
> Paolo