Re: [PATCH 4/4] block/vvfat: Avoid out of bounds write in create_long_filename()

[Eric Blake <eblake@redhat.com>]

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