[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/4] block/vvfat: Avoid out of bounds write in create_long_fi
From: |
Eric Blake |
Subject: |
Re: [PATCH 4/4] block/vvfat: Avoid out of bounds write in create_long_filename() |
Date: |
Wed, 5 May 2021 08:46:07 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 |
On 4/30/21 11:25 AM, Philippe Mathieu-Daudé wrote:
> The direntry_t::name holds 11 bytes:
>
> typedef struct direntry_t {
> uint8_t name[8 + 3];
> ...
struct direntry_t is poorly laid out. A quick google search finds:
https://www.ntfs.com/fat-filenames.htm
which shows that a long file name is indeed split over multiple fields
of struct direntry_t. In addition to the direntry_t for the 8+3 short
name, there are additional direntry_t reserved for each 13 bytes of the
long name (well, 26 bytes of UTF-16, as those 13 bytes are expanded into
Unicode).
>
> However create_long_filename() writes up to 31 bytes into it:
>
> 421 for(i=0;i<26*number_of_entries;i++) {
> 422 int offset=(i%26);
> 423 if(offset<10) offset=1+offset;
> 424 else if(offset<22) offset=14+offset-10;
> 425 else offset=28+offset-22;
> 426 entry=array_get(&(s->directory),s->directory.next-1-(i/26));
> 427 if (i >= 2 * length + 2) {
> 428 entry->name[offset] = 0xff;
> 429 } else if (i % 2 == 0) {
> 430 entry->name[offset] = longname[i / 2] & 0xff;
> 431 } else {
> 432 entry->name[offset] = longname[i / 2] >> 8;
> 433 }
> 434 }
The code is probably correct for which bytes it is writing, but wrong
for calling it entry->name[offset]; what we probably want is:
typedef struct direntry_t {
union {
struct {
uint8_t name[8 + 3];
...
uint32_t size;
};
char raw[32];
};
} QEMU_PACKED direntry_t;
where we then access through entry->raw[offset] instead of
entry->name[offset] (I know that anonymous union/structs are not
standard C, but I think that both gcc and clang support them, so that we
don't have to refactor the rest of the code to go through additional
names introduced by those modifications to direntry_t).
>
> For example, if i=25, offset=28+25-22=31
>
> Then in lines 428, 430 and 432 the entry->name[] array is written beside
> its 11 bytes, as reported by Clang sanitizer:
>
> block/vvfat.c:430:13: runtime error: index 14 out of bounds for type
> 'uint8_t [11]'
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> block/vvfat.c:430:13 in
> block/vvfat.c:432:13: runtime error: index 15 out of bounds for type
> 'uint8_t [11]'
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> block/vvfat.c:432:13 in
> block/vvfat.c:428:13: runtime error: index 18 out of bounds for type
> 'uint8_t [11]'
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> block/vvfat.c:428:13 in
>
> As I have no idea about what this code does, simply skip the writes if
> out of range, since it is not worst than what we have currently (and
> my tests using vvfat work identically).
>
> Fixes: de167e416fa ("Virtual VFAT support (Johannes Schindelin)")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
NACK. This violates what FAT expects for long names. Our code is not
pretty, but we should clean it up correctly rather than breaking it.
> ---
> block/vvfat.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/block/vvfat.c b/block/vvfat.c
> index c193a816646..c7162e77d68 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -423,6 +423,9 @@ static direntry_t *create_long_filename(BDRVVVFATState
> *s, const char *filename)
> if(offset<10) offset=1+offset;
> else if(offset<22) offset=14+offset-10;
> else offset=28+offset-22;
> + if (offset >= ARRAY_SIZE(entry->name)) {
> + continue;
> + }
> entry=array_get(&(s->directory),s->directory.next-1-(i/26));
> if (i >= 2 * length + 2) {
> entry->name[offset] = 0xff;
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH 4/4] block/vvfat: Avoid out of bounds write in create_long_filename(),
Eric Blake <=