[PATCH v3 0/4] Fix use-after-free and make format overflow more difficult

[PATCH v3 0/4] Fix use-after-free and make format overflow more difficult

[Akihiko Odaki]

nvme-ns has a use-after-free of a formatted string, so fix it by
embedding a fixed-length buffer to the object. Embedding a buffer lets
me avoid a chore to add a function to call g_free().

But I don't want to worry about a buffer overflow, so let the compiler
check that the buffer won't overflow; C is so restrictive that it cannot
enforce the existence of g_free(). Compilers can check the length of
formatted string on the other hand.

Then GCC started complaining about buffer overflow, so let's treat them.
Fortunately, the potential buffer overflows it detected are not
user-facing or very subtle. Treating them by growing buffers can improve
robustness with practically no cost.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Changes in v3:
- Changed to avoid sprintf() with "%.6f" in tests.
- Replaced the message of patch "vfio/pci: Grow buffer in
  vfio_pci_host_match()" with a improved version by Alex Williamson.
- Link to v2: https://lore.kernel.org/qemu-devel/20260302-nvme-v2-0-37ad8b5788c3@rsg.ci.i.u-tokyo.ac.jp

Changes in v2:
- Rebased.
- Changed to use g_strdup_printf() in patch
  "contrib/elf2dmp: Grow PDB URL buffer".
- Link to v1: https://lore.kernel.org/qemu-devel/20260125-nvme-v1-0-0658c31fade9@rsg.ci.i.u-tokyo.ac.jp

---
Akihiko Odaki (4):
      contrib/elf2dmp: Grow PDB URL buffer
      vfio/pci: Grow buffer in vfio_pci_host_match()
      tests: Avoid sprintf() with "%.6f"
      meson: Add -Wformat-overflow=2

 meson.build                              |  1 +
 contrib/elf2dmp/main.c                   | 32 +++++++++++++++-----------------
 hw/vfio/pci.c                            |  2 +-
 tests/unit/test-qobject-input-visitor.c  |  8 ++------
 tests/unit/test-qobject-output-visitor.c |  7 ++-----
 5 files changed, 21 insertions(+), 29 deletions(-)
---
base-commit: afe653676dc6dfd49f0390239ff90b2f0052c2b8
change-id: 20260125-nvme-b4661e0a409e

Best regards,
--  
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

[PATCH v3 1/4] contrib/elf2dmp: Grow PDB URL buffer

[Akihiko Odaki]

The buffers used to construct a PDB URL overflow when the "age" property
is greater than 0xf, so grow it. This also simplifies the logic of the
URL construction to use one buffer instead of two to avoid the chore to
synchronize the sizes of two buffers.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 contrib/elf2dmp/main.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
index d046a72ae67f..a62abadcc049 100644
--- a/contrib/elf2dmp/main.c
+++ b/contrib/elf2dmp/main.c
@@ -494,18 +494,6 @@ static bool pe_check_pdb_name(uint64_t base, void *start_addr,
     return !strcmp(pdb_name, PDB_NAME);
 }
 
-static void pe_get_pdb_symstore_hash(OMFSignatureRSDS *rsds, char *hash)
-{
-    sprintf(hash, "%.08x%.04x%.04x%.02x%.02x", rsds->guid.a, rsds->guid.b,
-            rsds->guid.c, rsds->guid.d[0], rsds->guid.d[1]);
-    hash += 20;
-    for (unsigned int i = 0; i < 6; i++, hash += 2) {
-        sprintf(hash, "%.02x", rsds->guid.e[i]);
-    }
-
-    sprintf(hash, "%.01x", rsds->age);
-}
-
 int main(int argc, char *argv[])
 {
     int err = 1;
@@ -517,9 +505,7 @@ int main(int argc, char *argv[])
     uint64_t KernBase;
     void *nt_start_addr = NULL;
     WinDumpHeader64 header;
-    char pdb_hash[34];
-    char pdb_url[] = SYM_URL_BASE PDB_NAME
-        "/0123456789ABCDEF0123456789ABCDEFx/" PDB_NAME;
+    g_autofree char *pdb_url = NULL;
     struct pdb_reader pdb;
     uint64_t KdDebuggerDataBlock;
     KDDEBUGGER_DATA64 *kdbg;
@@ -583,9 +569,21 @@ int main(int argc, char *argv[])
     printf("KernBase = 0x%016"PRIx64", signature is \'%.2s\'\n", KernBase,
             (char *)nt_start_addr);
 
-    pe_get_pdb_symstore_hash(&rsds, pdb_hash);
+    pdb_url = g_strdup_printf("%s"
+                              "%.08x%.04x%.04x"
+                              "%.02x%.02x"
+                              "%.02x%.02x"
+                              "%.02x%.02x"
+                              "%.02x%.02x%.01x"
+                              "%s",
+                              SYM_URL_BASE PDB_NAME "/",
+                              rsds.guid.a, rsds.guid.b, rsds.guid.c,
+                              rsds.guid.d[0], rsds.guid.d[1],
+                              rsds.guid.e[0], rsds.guid.e[1],
+                              rsds.guid.e[2], rsds.guid.e[3],
+                              rsds.guid.e[4], rsds.guid.e[5], rsds.age,
+                              "/" PDB_NAME);
 
-    sprintf(pdb_url, "%s%s/%s/%s", SYM_URL_BASE, PDB_NAME, pdb_hash, PDB_NAME);
     printf("PDB URL is %s\n", pdb_url);
 
     if (!download_url(PDB_NAME, pdb_url)) {

-- 
2.53.0

[PATCH v3 2/4] vfio/pci: Grow buffer in vfio_pci_host_match()

[Akihiko Odaki]

Each field of PCIHostDeviceAddress is an unsigned int, therefore
while a valid address is limited to 13 characters, an invalid
address could exceed the specified format, up to:

        ffffffff:ffffffff:ffffffff.ffffffff<NUL>

This requires 36 characters with the terminator.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Reviewed-by: Alex Williamson <alex.williamson@nvidia.com>
---
 hw/vfio/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c89f3fbea348..94c174a773fb 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2739,7 +2739,7 @@ void vfio_pci_post_reset(VFIOPCIDevice *vdev)
 
 bool vfio_pci_host_match(PCIHostDeviceAddress *addr, const char *name)
 {
-    char tmp[13];
+    char tmp[36];
 
     sprintf(tmp, "%04x:%02x:%02x.%1x", addr->domain,
             addr->bus, addr->slot, addr->function);

-- 
2.53.0

[PATCH v3 3/4] tests: Avoid sprintf() with "%.6f"

[Akihiko Odaki]

A string that represents a double can be long if it is an exponentially
large number.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 tests/unit/test-qobject-input-visitor.c  | 8 ++------
 tests/unit/test-qobject-output-visitor.c | 7 ++-----
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
index 84bdcdf702e0..beee11db4e47 100644
--- a/tests/unit/test-qobject-input-visitor.c
+++ b/tests/unit/test-qobject-input-visitor.c
@@ -500,7 +500,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
     g_string_append_printf(json, "'number': [");
     sep = "";
     for (i = 0; i < 32; i++) {
-        g_string_append_printf(json, "%s%f", sep, (double)i / 3);
+        g_string_append_printf(json, "%s%f", sep, (double)i / FLT_RADIX);
         sep = ", ";
     }
     g_string_append_printf(json, "], ");
@@ -583,11 +583,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
 
     i = 0;
     for (num_list = arrs->number; num_list; num_list = num_list->next) {
-        char expected[32], actual[32];
-
-        sprintf(expected, "%.6f", (double)i / 3);
-        sprintf(actual, "%.6f", num_list->value);
-        g_assert_cmpstr(expected, ==, actual);
+        g_assert_cmpfloat(num_list->value, ==, (double)i / FLT_RADIX);
         i++;
     }
 
diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c
index 407ab9ed505a..3c47b28f6638 100644
--- a/tests/unit/test-qobject-output-visitor.c
+++ b/tests/unit/test-qobject-output-visitor.c
@@ -538,7 +538,7 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
     }
 
     for (i = 31; i >= 0; i--) {
-        QAPI_LIST_PREPEND(arrs->number, (double)i / 3);
+        QAPI_LIST_PREPEND(arrs->number, (double)i / FLT_RADIX);
     }
 
     for (i = 31; i >= 0; i--) {
@@ -571,12 +571,9 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
     i = 0;
     QLIST_FOREACH_ENTRY(qlist, e) {
         QNum *qvalue = qobject_to(QNum, qlist_entry_obj(e));
-        char expected[32], actual[32];
 
         g_assert(qvalue);
-        sprintf(expected, "%.6f", (double)i / 3);
-        sprintf(actual, "%.6f", qnum_get_double(qvalue));
-        g_assert_cmpstr(actual, ==, expected);
+        g_assert_cmpfloat(qnum_get_double(qvalue), ==, (double)i / FLT_RADIX);
         i++;
     }
 

-- 
2.53.0

[PATCH v3 4/4] meson: Add -Wformat-overflow=2

[Akihiko Odaki]

https://gcc.gnu.org/onlinedocs/gcc-15.2.0/gcc/Warning-Options.html
> Level 2 warns also about calls that might overflow the destination
> buffer given an argument of sufficient length or magnitude. At level
> 2, unknown numeric arguments are assumed to have the minimum
> representable value for signed types with a precision greater than 1,
> and the maximum representable value otherwise. Unknown string
> arguments whose length cannot be assumed to be bounded either by the
> directive’s precision, or by a finite set of string literals they may
> evaluate to, or the character array they may point to, are assumed to
> be 1 character long.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 meson.build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/meson.build b/meson.build
index 414c8ea7e280..cf50bc492f9c 100644
--- a/meson.build
+++ b/meson.build
@@ -692,6 +692,7 @@ warn_flags = [
   '-Wempty-body',
   '-Wendif-labels',
   '-Wexpansion-to-defined',
+  '-Wformat-overflow=2',
   '-Wformat-security',
   '-Wformat-y2k',
   '-Wignored-qualifiers',

-- 
2.53.0

Re: [PATCH v3 3/4] tests: Avoid sprintf() with "%.6f"

[Markus Armbruster]

Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> writes:

> A string that represents a double can be long if it is an exponentially
> large number.

I'd like us to spell out that this is cleanup, not a bug fix.  I suggest
to start with the warning.  Something like this:

    tests: Clean up double comparisons to avoid compiler warning

    To enable -Wformat-overflow=2, we need to clean up a couple of false
    positives:

    <insert warnings here>

Now explain why they are false:

    These buffers cannot actually overflow because the doubles are
    between 0 and 31.0/3 inclusive.

Then justify the solution:

    However, formatting doubles just to compare them is silly.  Compare
    them directly instead.  To avoid potential rounding trouble, change
    the numbers tested to be representable exactly in double.

> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  tests/unit/test-qobject-input-visitor.c  | 8 ++------
>  tests/unit/test-qobject-output-visitor.c | 7 ++-----
>  2 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/tests/unit/test-qobject-input-visitor.c b/tests/unit/test-qobject-input-visitor.c
> index 84bdcdf702e0..beee11db4e47 100644
> --- a/tests/unit/test-qobject-input-visitor.c
> +++ b/tests/unit/test-qobject-input-visitor.c
> @@ -500,7 +500,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
>      g_string_append_printf(json, "'number': [");
>      sep = "";
>      for (i = 0; i < 32; i++) {
> -        g_string_append_printf(json, "%s%f", sep, (double)i / 3);
> +        g_string_append_printf(json, "%s%f", sep, (double)i / FLT_RADIX);
>          sep = ", ";
>      }
>      g_string_append_printf(json, "], ");
> @@ -583,11 +583,7 @@ static void test_visitor_in_list_struct(TestInputVisitorData *data,
>  
>      i = 0;
>      for (num_list = arrs->number; num_list; num_list = num_list->next) {
> -        char expected[32], actual[32];
> -
> -        sprintf(expected, "%.6f", (double)i / 3);
> -        sprintf(actual, "%.6f", num_list->value);
> -        g_assert_cmpstr(expected, ==, actual);
> +        g_assert_cmpfloat(num_list->value, ==, (double)i / FLT_RADIX);
>          i++;
>      }
>  
> diff --git a/tests/unit/test-qobject-output-visitor.c b/tests/unit/test-qobject-output-visitor.c
> index 407ab9ed505a..3c47b28f6638 100644
> --- a/tests/unit/test-qobject-output-visitor.c
> +++ b/tests/unit/test-qobject-output-visitor.c
> @@ -538,7 +538,7 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
>      }
>  
>      for (i = 31; i >= 0; i--) {
> -        QAPI_LIST_PREPEND(arrs->number, (double)i / 3);
> +        QAPI_LIST_PREPEND(arrs->number, (double)i / FLT_RADIX);
>      }
>  
>      for (i = 31; i >= 0; i--) {
> @@ -571,12 +571,9 @@ static void test_visitor_out_list_struct(TestOutputVisitorData *data,
>      i = 0;
>      QLIST_FOREACH_ENTRY(qlist, e) {
>          QNum *qvalue = qobject_to(QNum, qlist_entry_obj(e));
> -        char expected[32], actual[32];
>  
>          g_assert(qvalue);
> -        sprintf(expected, "%.6f", (double)i / 3);
> -        sprintf(actual, "%.6f", qnum_get_double(qvalue));
> -        g_assert_cmpstr(actual, ==, expected);
> +        g_assert_cmpfloat(qnum_get_double(qvalue), ==, (double)i / FLT_RADIX);
>          i++;
>      }

With an improved commit message
Reviewed-by: Markus Armbruster <armbru@redhat.com>