[PULL 00/11] Trivial patches for 2024-12-28

[PULL 00/11] Trivial patches for 2024-12-28

[Michael Tokarev]

The following changes since commit 38d0939b86e2eef6f6a622c6f1f7befda0146595:

  Merge tag 'pull-vfio-20241226' of https://github.com/legoater/qemu into staging (2024-12-26 04:38:38 -0500)

are available in the Git repository at:

  https://gitlab.com/mjt0k/qemu.git tags/pull-trivial-patches

for you to fetch changes up to 1e3d4d9a1a32ac6835f0d295a5117851c421fb5d:

  qmp: update vhost-user protocol feature maps (2024-12-28 14:42:53 +0300)

----------------------------------------------------------------
trivial patches for 2024-12-28

----------------------------------------------------------------
Daniel Henrique Barboza (1):
      hw/riscv/riscv-iommu-sys.c: fix duplicated 'table_size'

Han Han (1):
      target/i386/cpu: Fix notes for CPU models

Laurent Vivier (1):
      qmp: update vhost-user protocol feature maps

Philippe Mathieu-Daudé (3):
      docs: Correct '-runas' and '-fsdev/-virtfs proxy' indentation
      docs: Correct release of TCG trace-events removal
      docs: Replace 'since' -> 'removed in' in removed-features.rst

Pierrick Bouvier (2):
      docs/devel: remove dead video link for sourcehut submit process
      vvfat: fix ubsan issue in create_long_filename

Zhao Liu (2):
      hw/timer/hpet: Fix comment about capabilities register
      hw/timer/hpet: Drop the unused macro

ckf104 (1):
      contrib/plugins/bbv.c: Start bb index from 1

 block/vvfat.c                     |  4 ++++
 contrib/plugins/bbv.c             |  2 +-
 docs/about/deprecated.rst         |  2 +-
 docs/about/removed-features.rst   | 24 ++++++++++++------------
 docs/devel/submitting-a-patch.rst |  5 +----
 hw/riscv/riscv-iommu-sys.c        |  2 +-
 hw/timer/hpet.c                   |  2 +-
 hw/virtio/virtio-qmp.c            |  6 ++++++
 include/hw/timer/hpet.h           |  1 -
 target/i386/cpu.c                 |  3 ++-
 10 files changed, 29 insertions(+), 22 deletions(-)

[PULL 01/11] docs/devel: remove dead video link for sourcehut submit process

[Michael Tokarev]

From: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 docs/devel/submitting-a-patch.rst | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst
index 10b062eec2..03b2ac298a 100644
--- a/docs/devel/submitting-a-patch.rst
+++ b/docs/devel/submitting-a-patch.rst
@@ -252,10 +252,7 @@ patches to the QEMU mailing list by following these steps:
 #. Send your patches to the QEMU mailing list using the web-based
    ``git-send-email`` UI at https://git.sr.ht/~USERNAME/qemu/send-email
 
-`This video
-<https://spacepub.space/videos/watch/ad258d23-0ac6-488c-83fc-2bacf578de3a>`__
-shows the web-based ``git-send-email`` workflow. Documentation is
-available `here
+Documentation for sourcehut is available `here
 <https://man.sr.ht/git.sr.ht/#sending-patches-upstream>`__.
 
 .. _cc_the_relevant_maintainer:
-- 
2.39.5

[PULL 02/11] hw/timer/hpet: Fix comment about capabilities register

[Michael Tokarev]

From: Zhao Liu <zhao1.liu@intel.com>

HPETState.capability stores the emulated value for "general capabilities
and id register" instead of "main counter register".

Fix the comment to accurately reflect this.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/timer/hpet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 2a45410c0d..1c8c6c69ef 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -736,7 +736,7 @@ static void hpet_realize(DeviceState *dev, Error **errp)
         timer->state = s;
     }
 
-    /* 64-bit main counter; LegacyReplacementRoute. */
+    /* 64-bit General Capabilities and ID Register; LegacyReplacementRoute. */
     s->capability = 0x8086a001ULL;
     s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
     s->capability |= ((uint64_t)(HPET_CLK_PERIOD * FS_PER_NS) << 32);
-- 
2.39.5

[PULL 03/11] hw/timer/hpet: Drop the unused macro

[Michael Tokarev]

From: Zhao Liu <zhao1.liu@intel.com>

HPET_TN_CFG_BITS_READONLY_OR_RESERVED is not used in any place since
HPET_TN_CFG_WRITE_MASK has been already used to check and fix the
writable bits in hpet_ram_write().

Drop this unused macro.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 include/hw/timer/hpet.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/hw/timer/hpet.h b/include/hw/timer/hpet.h
index d17a8d4319..71e8c62453 100644
--- a/include/hw/timer/hpet.h
+++ b/include/hw/timer/hpet.h
@@ -58,7 +58,6 @@
 #define HPET_TN_CFG_WRITE_MASK  0x7f4e
 #define HPET_TN_INT_ROUTE_SHIFT      9
 #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
-#define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
 
 struct hpet_fw_entry
 {
-- 
2.39.5

[PULL 04/11] vvfat: fix ubsan issue in create_long_filename

[Michael Tokarev]

From: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Found with test sbsaref introduced in [1].

[1] https://patchew.org/QEMU/20241203213629.2482806-1-pierrick.bouvier@linaro.org/

../block/vvfat.c:433:24: runtime error: index 14 out of bounds for type 'uint8_t [11]'
    #0 0x56151a66b93a in create_long_filename ../block/vvfat.c:433
    #1 0x56151a66f3d7 in create_short_and_long_name ../block/vvfat.c:725
    #2 0x56151a670403 in read_directory ../block/vvfat.c:804
    #3 0x56151a674432 in init_directories ../block/vvfat.c:964
    #4 0x56151a67867b in vvfat_open ../block/vvfat.c:1258
    #5 0x56151a3b8e19 in bdrv_open_driver ../block.c:1660
    #6 0x56151a3bb666 in bdrv_open_common ../block.c:1985
    #7 0x56151a3cadb9 in bdrv_open_inherit ../block.c:4153
    #8 0x56151a3c8850 in bdrv_open_child_bs ../block.c:3731
    #9 0x56151a3ca832 in bdrv_open_inherit ../block.c:4098
    #10 0x56151a3cbe40 in bdrv_open ../block.c:4248
    #11 0x56151a46344f in blk_new_open ../block/block-backend.c:457
    #12 0x56151a388bd9 in blockdev_init ../blockdev.c:612
    #13 0x56151a38ab2d in drive_new ../blockdev.c:1006
    #14 0x5615190fca41 in drive_init_func ../system/vl.c:649
    #15 0x56151aa796dd in qemu_opts_foreach ../util/qemu-option.c:1135
    #16 0x5615190fd2b6 in configure_blockdev ../system/vl.c:708
    #17 0x56151910a307 in qemu_create_early_backends ../system/vl.c:2004
    #18 0x561519113fcf in qemu_init ../system/vl.c:3685
    #19 0x56151a7e438e in main ../system/main.c:47
    #20 0x7f72d1a46249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
    #21 0x7f72d1a46304 in __libc_start_main_impl ../csu/libc-start.c:360
    #22 0x561517e98510 in _start (/home/user/.work/qemu/build/qemu-system-aarch64+0x3b9b510)

The offset used can easily go beyond entry->name size. It's probably a
bug, but I don't have the time to dive into vfat specifics for now.

This change solves the ubsan issue, and is functionally equivalent, as
anything written past the entry->name array would not be read anyway.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 block/vvfat.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index 8ffe8b3b9b..f2eafaa923 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -426,6 +426,10 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
         else if(offset<22) offset=14+offset-10;
         else offset=28+offset-22;
         entry=array_get(&(s->directory),s->directory.next-1-(i/26));
+        /* ensure we don't write anything past entry->name */
+        if (offset >= sizeof(entry->name)) {
+            continue;
+        }
         if (i >= 2 * length + 2) {
             entry->name[offset] = 0xff;
         } else if (i % 2 == 0) {
-- 
2.39.5

[PULL 05/11] docs: Correct '-runas' and '-fsdev/-virtfs proxy' indentation

[Michael Tokarev]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

Use the same style for deprecated / removed commands.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 docs/about/deprecated.rst       | 2 +-
 docs/about/removed-features.rst | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 267892b62f..d6809f94ea 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -75,7 +75,7 @@ marked deprecated since 9.0, users have to ensure that all the topology members
 described with -smp are supported by the target machine.
 
 ``-runas`` (since 9.1)
-----------------------
+''''''''''''''''''''''
 
 Use ``-run-with user=..`` instead.
 
diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 9bebee795c..68fe0b47f9 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -518,7 +518,7 @@ and later do not support it because the virtio-scsi device was introduced for
 full SCSI support.  Use virtio-scsi instead when SCSI passthrough is required.
 
 ``-fsdev proxy`` and ``-virtfs proxy`` (since 9.2)
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+''''''''''''''''''''''''''''''''''''''''''''''''''
 
 The 9p ``proxy`` filesystem backend driver was originally developed to
 enhance security by dispatching low level filesystem operations from 9p
-- 
2.39.5

[PULL 06/11] docs: Correct release of TCG trace-events removal

[Michael Tokarev]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

TCG trace-events were deprecated before the v6.2 release,
and removed for v7.0.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 docs/about/removed-features.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index 68fe0b47f9..e3a87f3f55 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -1087,8 +1087,8 @@ processor IP (see `Intel discontinuance notification`_).
 TCG introspection features
 --------------------------
 
-TCG trace-events (since 6.2)
-''''''''''''''''''''''''''''
+TCG trace-events (removed in 7.0)
+'''''''''''''''''''''''''''''''''
 
 The ability to add new TCG trace points had bit rotted and as the
 feature can be replicated with TCG plugins it was removed. If
-- 
2.39.5

[PULL 07/11] docs: Replace 'since' -> 'removed in' in removed-features.rst

[Michael Tokarev]

From: Philippe Mathieu-Daudé <philmd@linaro.org>

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 docs/about/removed-features.rst | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/docs/about/removed-features.rst b/docs/about/removed-features.rst
index e3a87f3f55..cb1388049a 100644
--- a/docs/about/removed-features.rst
+++ b/docs/about/removed-features.rst
@@ -403,13 +403,13 @@ Sound card devices should be created using ``-device`` or ``-audio``.
 The exception is ``pcspk`` which can be activated using ``-machine
 pcspk-audiodev=<name>``.
 
-``-watchdog`` (since 7.2)
-'''''''''''''''''''''''''
+``-watchdog`` (removed in 7.2)
+''''''''''''''''''''''''''''''
 
 Use ``-device`` instead.
 
-Hexadecimal sizes with scaling multipliers (since 8.0)
-''''''''''''''''''''''''''''''''''''''''''''''''''''''
+Hexadecimal sizes with scaling multipliers (removed in 8.0)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
 Input parameters that take a size value should only use a size suffix
 (such as 'k' or 'M') when the base is written in decimal, and not when
@@ -510,15 +510,15 @@ than zero.
 
 Removed along with the ``compression`` migration capability.
 
-``-device virtio-blk,scsi=on|off`` (since 9.1)
-''''''''''''''''''''''''''''''''''''''''''''''
+``-device virtio-blk,scsi=on|off`` (removed in 9.1)
+'''''''''''''''''''''''''''''''''''''''''''''''''''
 
 The virtio-blk SCSI passthrough feature is a legacy VIRTIO feature.  VIRTIO 1.0
 and later do not support it because the virtio-scsi device was introduced for
 full SCSI support.  Use virtio-scsi instead when SCSI passthrough is required.
 
-``-fsdev proxy`` and ``-virtfs proxy`` (since 9.2)
-''''''''''''''''''''''''''''''''''''''''''''''''''
+``-fsdev proxy`` and ``-virtfs proxy`` (removed in 9.2)
+'''''''''''''''''''''''''''''''''''''''''''''''''''''''
 
 The 9p ``proxy`` filesystem backend driver was originally developed to
 enhance security by dispatching low level filesystem operations from 9p
@@ -532,8 +532,8 @@ security model option, or switch to ``virtiofs``.   The virtiofs daemon
 ``virtiofsd`` uses vhost to eliminate the high latency costs of the 9p
 ``proxy`` backend.
 
-``-portrait`` and ``-rotate`` (since 9.2)
-'''''''''''''''''''''''''''''''''''''''''
+``-portrait`` and ``-rotate`` (removed in 9.2)
+''''''''''''''''''''''''''''''''''''''''''''''
 
 The ``-portrait`` and ``-rotate`` options were documented as only
 working with the PXA LCD device, and all the machine types using
-- 
2.39.5

[PULL 08/11] target/i386/cpu: Fix notes for CPU models

[Michael Tokarev]

From: Han Han <hhan@redhat.com>

Fixes: 644e3c5d812 ("missing vmx features for Skylake-Server and Cascadelake-Server")
Signed-off-by: Han Han <hhan@redhat.com>
Reviewed-by: Chenyi Qiang <chenyi.qiang@intel.com>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 target/i386/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 660ddafc28..0b639848cd 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3692,6 +3692,7 @@ static const X86CPUDefinition builtin_x86_defs[] = {
             },
             {
                 .version = 4,
+                .note = "IBRS, EPT switching, no TSX",
                 .props = (PropValue[]) {
                     { "vmx-eptp-switching", "on" },
                     { /* end of list */ }
@@ -3826,7 +3827,7 @@ static const X86CPUDefinition builtin_x86_defs[] = {
               },
             },
             { .version = 4,
-              .note = "ARCH_CAPABILITIES, no TSX",
+              .note = "ARCH_CAPABILITIES, EPT switching, no TSX",
               .props = (PropValue[]) {
                   { "vmx-eptp-switching", "on" },
                   { /* end of list */ }
-- 
2.39.5

[PULL 09/11] hw/riscv/riscv-iommu-sys.c: fix duplicated 'table_size'

[Michael Tokarev]

From: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

Trivial fix for the following ticket:

CID 1568580:  Incorrect expression  (EVALUATION_ORDER)
In "table_size = table_size = n_vectors * 16U",
    "table_size" is written twice with the same value.

Cc: qemu-trivial@nongnu.org
Cc: Peter Maydell <peter.maydell@linaro.org>
Resolves: Coverity CID 1568580
Fixes: 01c1caa9d1 ("hw/riscv/virt.c, riscv-iommu-sys.c: add MSIx support")
Signed-off-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/riscv/riscv-iommu-sys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/riscv/riscv-iommu-sys.c b/hw/riscv/riscv-iommu-sys.c
index 28153f38da..65b24fb07d 100644
--- a/hw/riscv/riscv-iommu-sys.c
+++ b/hw/riscv/riscv-iommu-sys.c
@@ -121,7 +121,7 @@ static void riscv_iommu_sysdev_init_msi(RISCVIOMMUStateSys *s,
                                         uint32_t n_vectors)
 {
     RISCVIOMMUState *iommu = &s->iommu;
-    uint32_t table_size = table_size = n_vectors * PCI_MSIX_ENTRY_SIZE;
+    uint32_t table_size = n_vectors * PCI_MSIX_ENTRY_SIZE;
     uint32_t table_offset = RISCV_IOMMU_REG_MSI_CONFIG;
     uint32_t pba_size = QEMU_ALIGN_UP(n_vectors, 64) / 8;
     uint32_t pba_offset = RISCV_IOMMU_REG_MSI_CONFIG + 256;
-- 
2.39.5

[PULL 10/11] contrib/plugins/bbv.c: Start bb index from 1

[Michael Tokarev]

From: ckf104 <1900011634@pku.edu.cn>

Standard simpoint tool reqeusts that index of basic block index starts from 1.

Signed-off-by: ckf104 <1900011634@pku.edu.cn>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 contrib/plugins/bbv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/plugins/bbv.c b/contrib/plugins/bbv.c
index a5256517dd..b9da6f815e 100644
--- a/contrib/plugins/bbv.c
+++ b/contrib/plugins/bbv.c
@@ -109,7 +109,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
         bb = g_new(Bb, 1);
         bb->vaddr = vaddr;
         bb->count = qemu_plugin_scoreboard_new(sizeof(uint64_t));
-        bb->index = g_hash_table_size(bbs);
+        bb->index = g_hash_table_size(bbs) + 1;
         g_hash_table_replace(bbs, &bb->vaddr, bb);
     }
     g_rw_lock_writer_unlock(&bbs_lock);
-- 
2.39.5

[PULL 11/11] qmp: update vhost-user protocol feature maps

[Michael Tokarev]

From: Laurent Vivier <lvivier@redhat.com>

Add VHOST_USER_PROTOCOL_F_SHARED_OBJECT and
VHOST_USER_PROTOCOL_F_DEVICE_STATE protocol feature maps to
the virtio introspection.

Cc: jonah.palmer@oracle.com
Fixes: 160947666276 ("vhost-user: add shared_object msg")
Cc: aesteve@redhat.com
Fixes: cda83adc62b6 ("vhost-user: Interface for migration state transfer")
Cc: hreitz@redhat.com
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/virtio/virtio-qmp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
index cccc6fe761..8a32a3b105 100644
--- a/hw/virtio/virtio-qmp.c
+++ b/hw/virtio/virtio-qmp.c
@@ -121,6 +121,12 @@ static const qmp_virtio_feature_map_t vhost_user_protocol_map[] = {
     FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_STATUS, \
             "VHOST_USER_PROTOCOL_F_STATUS: Querying and notifying back-end "
             "device status supported"),
+    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_SHARED_OBJECT, \
+            "VHOST_USER_PROTOCOL_F_SHARED_OBJECT: Backend shared object "
+            "supported"),
+    FEATURE_ENTRY(VHOST_USER_PROTOCOL_F_DEVICE_STATE, \
+            "VHOST_USER_PROTOCOL_F_DEVICE_STATE: Backend device state transfer "
+            "supported"),
     { -1, "" }
 };
 
-- 
2.39.5

Re: [PULL 00/11] Trivial patches for 2024-12-28

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 116 bytes --]

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/10.0 for any user-visible changes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL 04/11] vvfat: fix ubsan issue in create_long_filename

[Volker Rümelin]

> Found with test sbsaref introduced in [1].
>
> [1] https://patchew.org/QEMU/20241203213629.2482806-1-pierrick.bouvier@linaro.org/
>
> ../block/vvfat.c:433:24: runtime error: index 14 out of bounds for type 'uint8_t [11]'
>     #0 0x56151a66b93a in create_long_filename ../block/vvfat.c:433
>     #1 0x56151a66f3d7 in create_short_and_long_name ../block/vvfat.c:725
>     #2 0x56151a670403 in read_directory ../block/vvfat.c:804
>     #3 0x56151a674432 in init_directories ../block/vvfat.c:964
>     #4 0x56151a67867b in vvfat_open ../block/vvfat.c:1258
>     #5 0x56151a3b8e19 in bdrv_open_driver ../block.c:1660
>     #6 0x56151a3bb666 in bdrv_open_common ../block.c:1985
>     #7 0x56151a3cadb9 in bdrv_open_inherit ../block.c:4153
>     #8 0x56151a3c8850 in bdrv_open_child_bs ../block.c:3731
>     #9 0x56151a3ca832 in bdrv_open_inherit ../block.c:4098
>     #10 0x56151a3cbe40 in bdrv_open ../block.c:4248
>     #11 0x56151a46344f in blk_new_open ../block/block-backend.c:457
>     #12 0x56151a388bd9 in blockdev_init ../blockdev.c:612
>     #13 0x56151a38ab2d in drive_new ../blockdev.c:1006
>     #14 0x5615190fca41 in drive_init_func ../system/vl.c:649
>     #15 0x56151aa796dd in qemu_opts_foreach ../util/qemu-option.c:1135
>     #16 0x5615190fd2b6 in configure_blockdev ../system/vl.c:708
>     #17 0x56151910a307 in qemu_create_early_backends ../system/vl.c:2004
>     #18 0x561519113fcf in qemu_init ../system/vl.c:3685
>     #19 0x56151a7e438e in main ../system/main.c:47
>     #20 0x7f72d1a46249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
>     #21 0x7f72d1a46304 in __libc_start_main_impl ../csu/libc-start.c:360
>     #22 0x561517e98510 in _start (/home/user/.work/qemu/build/qemu-system-aarch64+0x3b9b510)
>
> The offset used can easily go beyond entry->name size. It's probably a
> bug, but I don't have the time to dive into vfat specifics for now.

Hi Pierrick, Michael,

this patch breaks the creation of long filenames in the vvfat driver.

> This change solves the ubsan issue, and is functionally equivalent, as
> anything written past the entry->name array would not be read anyway.

This assumption is wrong. The guest reads the bytes written past the
entry->name array in the 32 byte direntry_t structure. A LFN direntry
structure is different from a regular direntry structure.

You patch limits the long file name to 5 UCS-2 characters out of
possible 13 UCS-2 characters per LFN entry.

To reproduce the issue:

On the host:
~> mkdir vvfat-drive
~> touch "vvfat-drive/file with a long name.txt"

and start QEMU with -blockdev
driver=vvfat,read-only=true,dir=./vvfat-drive,node-name=disk-d,label=hostdrive
-device scsi-hd,bus=scsi0.0,scsi-id=0,lun=1,drive=disk-d

On the guest without this patch:
~ # mount -t vfat -o ro /dev/sdb1 /mnt
~ # ls /mnt
file with a long name.txt

On the guest with this patch:
~ # mount -t vfat -o ro /dev/sdb1 /mnt
~ # ls /mnt
file
~ # ls /mnt | xxd
00000000: 6669 6c65 200a                           file .

With best regards,
Volker

>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  block/vvfat.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 8ffe8b3b9b..f2eafaa923 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -426,6 +426,10 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
>          else if(offset<22) offset=14+offset-10;
>          else offset=28+offset-22;
>          entry=array_get(&(s->directory),s->directory.next-1-(i/26));
> +        /* ensure we don't write anything past entry->name */
> +        if (offset >= sizeof(entry->name)) {
> +            continue;
> +        }
>          if (i >= 2 * length + 2) {
>              entry->name[offset] = 0xff;
>          } else if (i % 2 == 0) {

Re: [PULL 04/11] vvfat: fix ubsan issue in create_long_filename

[Michael Tokarev]

29.12.2024 12:24, Volker Rümelin wrote:
>> Found with test sbsaref introduced in [1].
>>
>> [1] https://patchew.org/QEMU/20241203213629.2482806-1-pierrick.bouvier@linaro.org/
>>
>> ../block/vvfat.c:433:24: runtime error: index 14 out of bounds for type 'uint8_t [11]'
>>      #0 0x56151a66b93a in create_long_filename ../block/vvfat.c:433
>>      #1 0x56151a66f3d7 in create_short_and_long_name ../block/vvfat.c:725
>>      #2 0x56151a670403 in read_directory ../block/vvfat.c:804
>>      #3 0x56151a674432 in init_directories ../block/vvfat.c:964
>>      #4 0x56151a67867b in vvfat_open ../block/vvfat.c:1258
>>      #5 0x56151a3b8e19 in bdrv_open_driver ../block.c:1660
>>      #6 0x56151a3bb666 in bdrv_open_common ../block.c:1985
>>      #7 0x56151a3cadb9 in bdrv_open_inherit ../block.c:4153
>>      #8 0x56151a3c8850 in bdrv_open_child_bs ../block.c:3731
>>      #9 0x56151a3ca832 in bdrv_open_inherit ../block.c:4098
>>      #10 0x56151a3cbe40 in bdrv_open ../block.c:4248
>>      #11 0x56151a46344f in blk_new_open ../block/block-backend.c:457
>>      #12 0x56151a388bd9 in blockdev_init ../blockdev.c:612
>>      #13 0x56151a38ab2d in drive_new ../blockdev.c:1006
>>      #14 0x5615190fca41 in drive_init_func ../system/vl.c:649
>>      #15 0x56151aa796dd in qemu_opts_foreach ../util/qemu-option.c:1135
>>      #16 0x5615190fd2b6 in configure_blockdev ../system/vl.c:708
>>      #17 0x56151910a307 in qemu_create_early_backends ../system/vl.c:2004
>>      #18 0x561519113fcf in qemu_init ../system/vl.c:3685
>>      #19 0x56151a7e438e in main ../system/main.c:47
>>      #20 0x7f72d1a46249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
>>      #21 0x7f72d1a46304 in __libc_start_main_impl ../csu/libc-start.c:360
>>      #22 0x561517e98510 in _start (/home/user/.work/qemu/build/qemu-system-aarch64+0x3b9b510)
>>
>> The offset used can easily go beyond entry->name size. It's probably a
>> bug, but I don't have the time to dive into vfat specifics for now.
> 
> Hi Pierrick, Michael,
> 
> this patch breaks the creation of long filenames in the vvfat driver.
> 
>> This change solves the ubsan issue, and is functionally equivalent, as
>> anything written past the entry->name array would not be read anyway.
> 
> This assumption is wrong. The guest reads the bytes written past the
> entry->name array in the 32 byte direntry_t structure. A LFN direntry
> structure is different from a regular direntry structure.
> 
> You patch limits the long file name to 5 UCS-2 characters out of
> possible 13 UCS-2 characters per LFN entry.

Yes, it looks like the original code was actually correct.  The LFN
entry uses other bytes in the directory struct to store the long file
name, scattered over the structure in a few places.

To my shame, I wrote LFN driver for MS-DOS at the time, and I definitely
knew how it laid out.  But this was.. many moons ago.

It looks like we should just revert this patch as incorrect.

In order to make the test happy, this code can be rewritten to use a
different approach.

/mjt

Re: [PULL 04/11] vvfat: fix ubsan issue in create_long_filename

[Pierrick Bouvier]

Hi Volker,

On 12/29/24 01:24, Volker Rümelin wrote:
>> Found with test sbsaref introduced in [1].
>>
>> [1] https://patchew.org/QEMU/20241203213629.2482806-1-pierrick.bouvier@linaro.org/
>>
>> ../block/vvfat.c:433:24: runtime error: index 14 out of bounds for type 'uint8_t [11]'
>>      #0 0x56151a66b93a in create_long_filename ../block/vvfat.c:433
>>      #1 0x56151a66f3d7 in create_short_and_long_name ../block/vvfat.c:725
>>      #2 0x56151a670403 in read_directory ../block/vvfat.c:804
>>      #3 0x56151a674432 in init_directories ../block/vvfat.c:964
>>      #4 0x56151a67867b in vvfat_open ../block/vvfat.c:1258
>>      #5 0x56151a3b8e19 in bdrv_open_driver ../block.c:1660
>>      #6 0x56151a3bb666 in bdrv_open_common ../block.c:1985
>>      #7 0x56151a3cadb9 in bdrv_open_inherit ../block.c:4153
>>      #8 0x56151a3c8850 in bdrv_open_child_bs ../block.c:3731
>>      #9 0x56151a3ca832 in bdrv_open_inherit ../block.c:4098
>>      #10 0x56151a3cbe40 in bdrv_open ../block.c:4248
>>      #11 0x56151a46344f in blk_new_open ../block/block-backend.c:457
>>      #12 0x56151a388bd9 in blockdev_init ../blockdev.c:612
>>      #13 0x56151a38ab2d in drive_new ../blockdev.c:1006
>>      #14 0x5615190fca41 in drive_init_func ../system/vl.c:649
>>      #15 0x56151aa796dd in qemu_opts_foreach ../util/qemu-option.c:1135
>>      #16 0x5615190fd2b6 in configure_blockdev ../system/vl.c:708
>>      #17 0x56151910a307 in qemu_create_early_backends ../system/vl.c:2004
>>      #18 0x561519113fcf in qemu_init ../system/vl.c:3685
>>      #19 0x56151a7e438e in main ../system/main.c:47
>>      #20 0x7f72d1a46249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
>>      #21 0x7f72d1a46304 in __libc_start_main_impl ../csu/libc-start.c:360
>>      #22 0x561517e98510 in _start (/home/user/.work/qemu/build/qemu-system-aarch64+0x3b9b510)
>>
>> The offset used can easily go beyond entry->name size. It's probably a
>> bug, but I don't have the time to dive into vfat specifics for now.
> 
> Hi Pierrick, Michael,
> 
> this patch breaks the creation of long filenames in the vvfat driver.
> 
>> This change solves the ubsan issue, and is functionally equivalent, as
>> anything written past the entry->name array would not be read anyway.
> 
> This assumption is wrong. The guest reads the bytes written past the
> entry->name array in the 32 byte direntry_t structure. A LFN direntry
> structure is different from a regular direntry structure.
> 
> You patch limits the long file name to 5 UCS-2 characters out of
> possible 13 UCS-2 characters per LFN entry.
> 
> To reproduce the issue:
> 
> On the host:
> ~> mkdir vvfat-drive
> ~> touch "vvfat-drive/file with a long name.txt"
> 
> and start QEMU with -blockdev
> driver=vvfat,read-only=true,dir=./vvfat-drive,node-name=disk-d,label=hostdrive
> -device scsi-hd,bus=scsi0.0,scsi-id=0,lun=1,drive=disk-d
> 
> On the guest without this patch:
> ~ # mount -t vfat -o ro /dev/sdb1 /mnt
> ~ # ls /mnt
> file with a long name.txt
> 
> On the guest with this patch:
> ~ # mount -t vfat -o ro /dev/sdb1 /mnt
> ~ # ls /mnt
> file
> ~ # ls /mnt | xxd
> 00000000: 6669 6c65 200a                           file .
> 

Thanks very much for analyzing and fixing the underlying issue Volker.

I was not sure what was happening here, but doing memory aliasing with 
two different structs is definitely a suspicious pattern.
I'll test the patch you sent, and give a review once I check it works.

Regards,
Pierrick

> With best regards,
> Volker
> 
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> Reviewed-by: Michael Tokarev <mjt@tls.msk.ru>
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>> ---
>>   block/vvfat.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 8ffe8b3b9b..f2eafaa923 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -426,6 +426,10 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
>>           else if(offset<22) offset=14+offset-10;
>>           else offset=28+offset-22;
>>           entry=array_get(&(s->directory),s->directory.next-1-(i/26));
>> +        /* ensure we don't write anything past entry->name */
>> +        if (offset >= sizeof(entry->name)) {
>> +            continue;
>> +        }
>>           if (i >= 2 * length + 2) {
>>               entry->name[offset] = 0xff;
>>           } else if (i % 2 == 0) {
>