block/vvfat.c | 55 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 14 deletions(-)
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>
---
block/vvfat.c | 55 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 41 insertions(+), 14 deletions(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 8ffe8b3b9b..626c4f0163 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -255,6 +255,17 @@ typedef struct direntry_t {
uint32_t size;
} QEMU_PACKED direntry_t;
+typedef struct lfn_direntry_t {
+ uint8_t sequence;
+ uint8_t name01[10];
+ uint8_t attributes;
+ uint8_t direntry_type;
+ uint8_t sfn_checksum;
+ uint8_t name0e[12];
+ uint16_t begin;
+ uint8_t name1c[4];
+} QEMU_PACKED lfn_direntry_t;
+
/* this structure are used to transparently access the files */
typedef struct mapping_t {
@@ -399,11 +410,28 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs)
/* direntry functions */
+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;
+ }
+}
+
static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
{
int number_of_entries, i;
glong length;
- direntry_t *entry;
+ lfn_direntry_t *entry;
gunichar2 *longname = g_utf8_to_utf16(filename, -1, NULL, &length, NULL);
if (!longname) {
@@ -415,24 +443,23 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
for(i=0;i<number_of_entries;i++) {
entry=array_get_next(&(s->directory));
+ entry->sequence = (number_of_entries - i) | (i == 0 ? 0x40 : 0);
entry->attributes=0xf;
- entry->reserved[0]=0;
+ entry->direntry_type = 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));
+ }
+ for (i = 0; i < 26 * number_of_entries; i++) {
+ uint8_t 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);
}
g_free(longname);
return array_get(&(s->directory),s->directory.next-number_of_entries);
@@ -731,7 +758,7 @@ static inline direntry_t* create_short_and_long_name(BDRVVVFATState* s,
/* calculate anew, because realloc could have taken place */
entry_long=array_get(&(s->directory),long_index);
while(entry_long<entry && is_long_name(entry_long)) {
- entry_long->reserved[1]=chksum;
+ ((lfn_direntry_t *)entry_long)->sfn_checksum = chksum;
entry_long++;
}
}
--
2.43.0
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);
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
22.01.2025 02:14, Pierrick Bouvier wrote: .. > 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. For an actual *fix*, please take a look at https://lore.kernel.org/qemu-devel/20250119093233.9E4C450B6D@localhost.tls.msk.ru/ which is minimal, understandable, verified and works. /mjt
On Wed, 22 Jan 2025, Michael Tokarev wrote: > 22.01.2025 02:14, Pierrick Bouvier wrote: > .. >> 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. > > For an actual *fix*, please take a look at > https://lore.kernel.org/qemu-devel/20250119093233.9E4C450B6D@localhost.tls.msk.ru/ > > which is minimal, understandable, verified and works. Just noticed in that patch you have several &(s->directory) where () is not needed, -> is higher priority than & (address_of). Regards, BALATON Zoltan
22.01.2025 15:19, BALATON Zoltan wrote: > On Wed, 22 Jan 2025, Michael Tokarev wrote: >> 22.01.2025 02:14, Pierrick Bouvier wrote: >> .. >>> 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. >> >> For an actual *fix*, please take a look at >> https://lore.kernel.org/qemu-devel/20250119093233.9E4C450B6D@localhost.tls.msk.ru/ >> >> which is minimal, understandable, verified and works. > > Just noticed in that patch you have several &(s->directory) where () is not needed, -> is higher priority than & (address_of). Yes. I especially mentioned that I kept the original style, to minimize the changes. It is not needed to fix the issue at hand, the fix is maximally targeted (or minimally). The subsequent patch - which is optional, unrelated to the issue at hand - changes all that stuff to adhere to qemu coding style (and yes, this is a style thing, for some, these parens makes it more readable). Thanks, /mjt
On 1/21/25 19:47, Michael Tokarev wrote: > 22.01.2025 02:14, Pierrick Bouvier wrote: > .. >> 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. > > For an actual *fix*, please take a look at > https://lore.kernel.org/qemu-devel/20250119093233.9E4C450B6D@localhost.tls.msk.ru/ > > which is minimal, understandable, verified and works. > > /mjt Thanks, I was not sure what to choose between the different patches posted around this, to fix the problem on my personal tree.
Hi Volker,
On 1/5/25 05: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>
> ---
> block/vvfat.c | 55 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 8ffe8b3b9b..626c4f0163 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -255,6 +255,17 @@ typedef struct direntry_t {
> uint32_t size;
> } QEMU_PACKED direntry_t;
>
> +typedef struct lfn_direntry_t {
> + uint8_t sequence;
> + uint8_t name01[10];
> + uint8_t attributes;
> + uint8_t direntry_type;
> + uint8_t sfn_checksum;
> + uint8_t name0e[12];
> + uint16_t begin;
> + uint8_t name1c[4];
> +} QEMU_PACKED lfn_direntry_t;
> +
> /* this structure are used to transparently access the files */
>
> typedef struct mapping_t {
> @@ -399,11 +410,28 @@ static void init_mbr(BDRVVVFATState *s, int cyls, int heads, int secs)
>
> /* direntry functions */
>
> +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;
> + }
> +}
> +
> static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
> {
> int number_of_entries, i;
> glong length;
> - direntry_t *entry;
> + lfn_direntry_t *entry;
>
> gunichar2 *longname = g_utf8_to_utf16(filename, -1, NULL, &length, NULL);
> if (!longname) {
> @@ -415,24 +443,23 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
>
> for(i=0;i<number_of_entries;i++) {
> entry=array_get_next(&(s->directory));
> + entry->sequence = (number_of_entries - i) | (i == 0 ? 0x40 : 0);
> entry->attributes=0xf;
> - entry->reserved[0]=0;
> + entry->direntry_type = 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));
> + }
> + for (i = 0; i < 26 * number_of_entries; i++) {
> + uint8_t 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);
> }
> g_free(longname);
> return array_get(&(s->directory),s->directory.next-number_of_entries);
> @@ -731,7 +758,7 @@ static inline direntry_t* create_short_and_long_name(BDRVVVFATState* s,
> /* calculate anew, because realloc could have taken place */
> entry_long=array_get(&(s->directory),long_index);
> while(entry_long<entry && is_long_name(entry_long)) {
> - entry_long->reserved[1]=chksum;
> + ((lfn_direntry_t *)entry_long)->sfn_checksum = chksum;
> entry_long++;
> }
> }
I tested this patch and it correctly fixes the issue reported by ubsan,
while keeping the feature working as expected.
Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Thus, it can be merged (cc to qemu-trivial for quick integration).
Thank you Volker,
Pierrick
© 2016 - 2025 Red Hat, Inc.