qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] vvfat: fix out of bounds array write


From: Pierrick Bouvier
Subject: Re: [PATCH] vvfat: fix out of bounds array write
Date: Tue, 21 Jan 2025 15:14:14 -0800
User-agent: Mozilla Thunderbird

On 1/18/25 09:10, Michael Tokarev wrote:
05.01.2025 16:59, Volker Rümelin wrote:
In function create_long_filname(), the array name[8 + 3] in
struct direntry_t is used as if it were defined as name[32].
This is intentional and works. It's nevertheless an out of
bounds array access. To avoid this problem, this patch adds a
struct lfn_direntry_t with multiple name arrays. A directory
entry for a long FAT file name is significantly different from
a directory entry for a regular FAT file name.

Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>


+static void write_long_filename(lfn_direntry_t *entry, int index, uint8_t c)
+{
+    if (index < ARRAY_SIZE(entry->name01)) {
+        entry->name01[index] = c;
+        return;
+    }
+    index -= ARRAY_SIZE(entry->name01);
+    if (index < ARRAY_SIZE(entry->name0e)) {
+        entry->name0e[index] = c;
+        return;
+    }
+    index -= ARRAY_SIZE(entry->name0e);
+    if (index < ARRAY_SIZE(entry->name1c)) {
+        entry->name1c[index] = c;
+    }
+}



+        entry = array_get(&s->directory, s->directory.next - i / 26 - 1);
           if (i >= 2 * length + 2) {
-            entry->name[offset] = 0xff;
+            c = 0xff;
           } else if (i % 2 == 0) {
-            entry->name[offset] = longname[i / 2] & 0xff;
+            c = longname[i / 2] & 0xff;
           } else {
-            entry->name[offset] = longname[i / 2] >> 8;
+            c = longname[i / 2] >> 8;
           }
+        write_long_filename(entry, i % 26, c);

Ghrm.  This doubles the twisty "if" statement...  :(

How about this instead (untested)?

The idea is to iterate over all dentries once, and fill each part inside the
dentry structure in turn, going on by utf16 char, instead by bytes/offset.

I think it is a bit clearer...

@@ -399,11 +410,21 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int 
heads, int secs)

   /* direntry functions */

+static unsigned write_long_filename_part(uint8_t *dest, unsigned dsize,
+                                         const gunichar2 *name, unsigned nlen)
+{
+    unsigned i;
+    for(i = 0; i < dsize / 2 && i < nlen; ++i) {
+        dest[i / 2 + 0] = name[i] & 0xff;
+        dest[i / 2 + 1] = name[i] >> 8;
+    }
+    return i;
+}
+
   static direntry_t *create_long_filename(BDRVVVFATState *s, const char 
*filename)
   {
-    int number_of_entries, i;
+    unsigned number_of_entries, ei, ci;
       glong length;
-    direntry_t *entry;

       gunichar2 *longname = g_utf8_to_utf16(filename, -1, NULL, &length, NULL);
       if (!longname) {
@@ -411,28 +432,21 @@ static direntry_t *create_long_filename(BDRVVVFATState 
*s, const char *filename)
           return NULL;
       }

-    number_of_entries = DIV_ROUND_UP(length * 2, 26);
+    /* each lfn_direntry holds 13 utf16 chars (26 bytes) */
+    number_of_entries = DIV_ROUND_UP(length, 13);

-    for(i=0;i<number_of_entries;i++) {
-        entry=array_get_next(&(s->directory));
-        entry->attributes=0xf;
-        entry->reserved[0]=0;
-        entry->begin=0;
-        entry->name[0]=(number_of_entries-i)|(i==0?0x40:0);
-    }
-    for(i=0;i<26*number_of_entries;i++) {
-        int offset=(i%26);
-        if(offset<10) offset=1+offset;
-        else if(offset<22) offset=14+offset-10;
-        else offset=28+offset-22;
-        entry=array_get(&(s->directory),s->directory.next-1-(i/26));
-        if (i >= 2 * length + 2) {
-            entry->name[offset] = 0xff;
-        } else if (i % 2 == 0) {
-            entry->name[offset] = longname[i / 2] & 0xff;
-        } else {
-            entry->name[offset] = longname[i / 2] >> 8;
-        }
+    for(ei = 0, ci = 0; i < number_of_entries; ei++) {
+        lfn_direntry_t *entry = array_get_next(&(s->directory));
+        entry->sequence = (number_of_entries - ei) | (ei == 0 ? 0x40 : 0);
+        entry->attributes = 0xf;
+        entry->direntry_type = 0;
+        entry->begin = 0;
+        ci += write_long_filename_part(entry->name01, sizeof(entry->name01),
+                                       longname + ci, length - ci);
+        ci += write_long_filename_part(entry->name0e, sizeof(entry->name0e),
+                                       longname + ci, length - ci);
+        ci += write_long_filename_part(entry->name1c, sizeof(entry->name1c),
+                                       longname + ci, length - ci);
       }
       g_free(longname);
       return array_get(&(s->directory),s->directory.next-number_of_entries);


I agree the existing code (and this patch) is pretty cryptic for anyone not familiar with FAT format. However, I think it could be a good thing to first merge this one (which is correct, and works), and refactor this in a second time, so the current ubsan issue is fixed upstream as soon as possible.
Note: the test triggering it has already been merged.

Regards,
Pierrick

reply via email to

[Prev in Thread] Current Thread [Next in Thread]