| Message ID | 20241228115446.2478706-5-mjt@tls.msk.ru |
|---|---|
| State | Accepted |
| Commit | 0cb3ff7c22671aa1e1e227318799ccf6762c3bea |
| Headers | show |
| Series | [PULL,01/11] docs/devel: remove dead video link for sourcehut submit process | expand |
> 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) {
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
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) {