[PATCH 0/2] vvfat: Two small patches

[PATCH 0/2] vvfat: Two small patches

[Kevin Wolf]

This fixes two of the four cases (1. and 4.) reported in:

    https://bugs.launchpad.net/qemu/+bug/1883083

The third case I attempted to fix looks a bit more complicated and I
don't have the time right now to learn how vvfat really works, so let's
merge these two patches at least.

Kevin Wolf (2):
  vvfat: Check that updated filenames are valid
  vvfat: Fix array_remove_slice()

 block/vvfat.c | 65 ++++++++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 37 deletions(-)

-- 
2.25.4

[PATCH 1/2] vvfat: Check that updated filenames are valid

[Kevin Wolf]

FAT allows only a restricted set of characters in file names, and for
some of the illegal characters, it's actually important that we catch
them: If filenames can contain '/', the guest can construct filenames
containing "../" and escape from the assigned vvfat directory. The same
problem could arise if ".." was ever accepted as a literal filename.

Fix this by adding a check that all filenames are valid in
check_directory_consistency().

Reported-by: Nathan Huckleberry <nhuck15@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index c65a98e3ee..2fab371258 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -520,6 +520,25 @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin)
     direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff);
 }
 
+static bool valid_filename(const unsigned char *name)
+{
+    unsigned char c;
+    if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) {
+        return false;
+    }
+    for (; (c = *name); name++) {
+        if (!((c >= '0' && c <= '9') ||
+              (c >= 'A' && c <= 'Z') ||
+              (c >= 'a' && c <= 'z') ||
+              c > 127 ||
+              strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != 0))
+        {
+            return false;
+        }
+    }
+    return true;
+}
+
 static uint8_t to_valid_short_char(gunichar c)
 {
     c = g_unichar_toupper(c);
@@ -2098,6 +2117,10 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))
             }
             lfn.checksum = 0x100; /* cannot use long name twice */
 
+            if (!valid_filename(lfn.name)) {
+                fprintf(stderr, "Invalid file name\n");
+                goto fail;
+            }
             if (path_len + 1 + lfn.len >= PATH_MAX) {
                 fprintf(stderr, "Name too long: %s/%s\n", path, lfn.name);
                 goto fail;
-- 
2.25.4

[PATCH 2/2] vvfat: Fix array_remove_slice()

[Kevin Wolf]

array_remove_slice() calls array_roll() with array->next - 1 as the
destination index. This is only correct for count == 1, otherwise we're
writing past the end of the array. array->next - count would be correct.

However, this is the only place ever calling array_roll(), so this
rather complicated operation isn't even necessary.

Fix the problem and simplify the code by replacing it with a single
memmove() call. array_roll() can now be removed.

Reported-by: Nathan Huckleberry <nhuck15@gmail.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/vvfat.c | 42 +++++-------------------------------------
 1 file changed, 5 insertions(+), 37 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 2fab371258..d6e464c595 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int
     return array->pointer+index*array->item_size;
 }
 
-/* this performs a "roll", so that the element which was at index_from becomes
- * index_to, but the order of all other elements is preserved. */
-static inline int array_roll(array_t* array,int index_to,int index_from,int count)
-{
-    char* buf;
-    char* from;
-    char* to;
-    int is;
-
-    if(!array ||
-            index_to<0 || index_to>=array->next ||
-            index_from<0 || index_from>=array->next)
-        return -1;
-
-    if(index_to==index_from)
-        return 0;
-
-    is=array->item_size;
-    from=array->pointer+index_from*is;
-    to=array->pointer+index_to*is;
-    buf=g_malloc(is*count);
-    memcpy(buf,from,is*count);
-
-    if(index_to<index_from)
-        memmove(to+is*count,to,from-to);
-    else
-        memmove(from,from+is*count,to-from);
-
-    memcpy(to,buf,is*count);
-
-    g_free(buf);
-
-    return 0;
-}
-
 static inline int array_remove_slice(array_t* array,int index, int count)
 {
     assert(index >=0);
     assert(count > 0);
     assert(index + count <= array->next);
-    if(array_roll(array,array->next-1,index,count))
-        return -1;
+
+    memmove(array->pointer + index * array->item_size,
+            array->pointer + (index + count) * array->item_size,
+            (array->next - index - count) * array->item_size);
+
     array->next -= count;
     return 0;
 }
-- 
2.25.4

Re: [PATCH 1/2] vvfat: Check that updated filenames are valid

[Eric Blake]

On 6/23/20 12:55 PM, Kevin Wolf wrote:
> FAT allows only a restricted set of characters in file names, and for
> some of the illegal characters, it's actually important that we catch
> them: If filenames can contain '/', the guest can construct filenames
> containing "../" and escape from the assigned vvfat directory. The same
> problem could arise if ".." was ever accepted as a literal filename.
> 
> Fix this by adding a check that all filenames are valid in
> check_directory_consistency().
> 
> Reported-by: Nathan Huckleberry <nhuck15@gmail.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vvfat.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index c65a98e3ee..2fab371258 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -520,6 +520,25 @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin)
>       direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff);
>   }
>   
> +static bool valid_filename(const unsigned char *name)
> +{
> +    unsigned char c;
> +    if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) {
> +        return false;
> +    }
> +    for (; (c = *name); name++) {
> +        if (!((c >= '0' && c <= '9') ||
> +              (c >= 'A' && c <= 'Z') ||
> +              (c >= 'a' && c <= 'z') ||
> +              c > 127 ||
> +              strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != 0))

s/0/NULL/

Hmm - would it be any more efficient to use a single comparison of 
strcspn() vs. strlen(), where you merely spell out the bytes that are 
rejected?  Out of 256 byte values, NUL is implicitly rejected (since 
these are C strings), the 128 high-bit bytes are all valid, and you have 
permitted 62 alnum and 23 other characters; that leaves merely 42 byte 
values to explicitly list in a reject string.  Of course, writing the 
string literal containing those 42 invalid bytes is itself a bit of an 
exercise in reading the ASCII table:

"\x01\x02\x03\x04\x05\x06\x07"
"\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f"
"\x10\x11\x12\x13\x14\x15\x16\x17"
"\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f"
" \"*/:<>?\\|\x7f"

> +        {
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
>   static uint8_t to_valid_short_char(gunichar c)
>   {
>       c = g_unichar_toupper(c);
> @@ -2098,6 +2117,10 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i))
>               }
>               lfn.checksum = 0x100; /* cannot use long name twice */
>   
> +            if (!valid_filename(lfn.name)) {
> +                fprintf(stderr, "Invalid file name\n");

Wow, the fact that we are still using fprintf is annoying, but pre-existing.

> +                goto fail;
> +            }
>               if (path_len + 1 + lfn.len >= PATH_MAX) {
>                   fprintf(stderr, "Name too long: %s/%s\n", path, lfn.name);
>                   goto fail;
> 

At any rate, the idea makes sense. If you don't like my strcspn() idea, 
then:
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/2] vvfat: Fix array_remove_slice()

[Eric Blake]

On 6/23/20 12:55 PM, Kevin Wolf wrote:
> array_remove_slice() calls array_roll() with array->next - 1 as the
> destination index. This is only correct for count == 1, otherwise we're
> writing past the end of the array. array->next - count would be correct.
> 
> However, this is the only place ever calling array_roll(), so this
> rather complicated operation isn't even necessary.
> 
> Fix the problem and simplify the code by replacing it with a single
> memmove() call. array_roll() can now be removed.
> 
> Reported-by: Nathan Huckleberry <nhuck15@gmail.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/vvfat.c | 42 +++++-------------------------------------
>   1 file changed, 5 insertions(+), 37 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 2fab371258..d6e464c595 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int
>       return array->pointer+index*array->item_size;
>   }
>   
> -/* this performs a "roll", so that the element which was at index_from becomes
> - * index_to, but the order of all other elements is preserved. */
> -static inline int array_roll(array_t* array,int index_to,int index_from,int count)

If I understand the intent from just the comment, the old code would 
take a directory listing of six files:

ABCDEF

and on the request to delete file C, would produce:

ABFDE

by moving just F, instead of all of DEF.  That might be what legacy FAT 
filesystems do natively, but I don't see it as a mandatory correctness 
issue; ABDEF is still a directory with the same contents.  And the bug 
for reading beyond array bounds when deleting more than one file is 
indeed nasty, so your simpler code is fine.

>   static inline int array_remove_slice(array_t* array,int index, int count)
>   {
>       assert(index >=0);
>       assert(count > 0);
>       assert(index + count <= array->next);
> -    if(array_roll(array,array->next-1,index,count))
> -        return -1;
> +
> +    memmove(array->pointer + index * array->item_size,
> +            array->pointer + (index + count) * array->item_size,
> +            (array->next - index - count) * array->item_size);
> +

Reviewed-by: Eric Blake <eblake@redhat.com>

>       array->next -= count;
>       return 0;
>   }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/2] vvfat: Two small patches

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200623175534.38286-1-kwolf@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH 0/2] vvfat: Two small patches
Type: series
Message-id: 20200623175534.38286-1-kwolf@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200623175534.38286-1-kwolf@redhat.com -> patchew/20200623175534.38286-1-kwolf@redhat.com
Switched to a new branch 'test'
5d9f5f1 vvfat: Fix array_remove_slice()
f1dd8bf vvfat: Check that updated filenames are valid

=== OUTPUT BEGIN ===
1/2 Checking commit f1dd8bf731ce (vvfat: Check that updated filenames are valid)
ERROR: "(foo*)" should be "(foo *)"
#31: FILE: block/vvfat.c:526:
+    if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) {

total: 1 errors, 0 warnings, 35 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit 5d9f5f1638af (vvfat: Fix array_remove_slice())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200623175534.38286-1-kwolf@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 1/2] vvfat: Check that updated filenames are valid

[Kevin Wolf]

Am 23.06.2020 um 20:21 hat Eric Blake geschrieben:
> On 6/23/20 12:55 PM, Kevin Wolf wrote:
> > FAT allows only a restricted set of characters in file names, and for
> > some of the illegal characters, it's actually important that we catch
> > them: If filenames can contain '/', the guest can construct filenames
> > containing "../" and escape from the assigned vvfat directory. The same
> > problem could arise if ".." was ever accepted as a literal filename.
> > 
> > Fix this by adding a check that all filenames are valid in
> > check_directory_consistency().
> > 
> > Reported-by: Nathan Huckleberry <nhuck15@gmail.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/vvfat.c | 23 +++++++++++++++++++++++
> >   1 file changed, 23 insertions(+)
> > 
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index c65a98e3ee..2fab371258 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -520,6 +520,25 @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin)
> >       direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff);
> >   }
> > +static bool valid_filename(const unsigned char *name)
> > +{
> > +    unsigned char c;
> > +    if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) {
> > +        return false;
> > +    }
> > +    for (; (c = *name); name++) {
> > +        if (!((c >= '0' && c <= '9') ||
> > +              (c >= 'A' && c <= 'Z') ||
> > +              (c >= 'a' && c <= 'z') ||
> > +              c > 127 ||
> > +              strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != 0))
> 
> s/0/NULL/

Ok, though this line is just copied from to_valid_short_char(). Maybe I
can sneak in a (strictly speaking unrelated) change to that function to
keep both consistent.

> Hmm - would it be any more efficient to use a single comparison of strcspn()
> vs. strlen(), where you merely spell out the bytes that are rejected?  Out
> of 256 byte values, NUL is implicitly rejected (since these are C strings),
> the 128 high-bit bytes are all valid, and you have permitted 62 alnum and 23
> other characters; that leaves merely 42 byte values to explicitly list in a
> reject string.  Of course, writing the string literal containing those 42
> invalid bytes is itself a bit of an exercise in reading the ASCII table:
> 
> "\x01\x02\x03\x04\x05\x06\x07"
> "\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f"
> "\x10\x11\x12\x13\x14\x15\x16\x17"
> "\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f"
> " \"*/:<>?\\|\x7f"

I think this would be really hard to read.

The above condition is a pretty straighforward implementation of what
the spec says (even the order of characters is the same).

Kevin

Re: [PATCH 2/2] vvfat: Fix array_remove_slice()

[Kevin Wolf]

Am 23.06.2020 um 20:30 hat Eric Blake geschrieben:
> On 6/23/20 12:55 PM, Kevin Wolf wrote:
> > array_remove_slice() calls array_roll() with array->next - 1 as the
> > destination index. This is only correct for count == 1, otherwise we're
> > writing past the end of the array. array->next - count would be correct.
> > 
> > However, this is the only place ever calling array_roll(), so this
> > rather complicated operation isn't even necessary.
> > 
> > Fix the problem and simplify the code by replacing it with a single
> > memmove() call. array_roll() can now be removed.
> > 
> > Reported-by: Nathan Huckleberry <nhuck15@gmail.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/vvfat.c | 42 +++++-------------------------------------
> >   1 file changed, 5 insertions(+), 37 deletions(-)
> > 
> > diff --git a/block/vvfat.c b/block/vvfat.c
> > index 2fab371258..d6e464c595 100644
> > --- a/block/vvfat.c
> > +++ b/block/vvfat.c
> > @@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int
> >       return array->pointer+index*array->item_size;
> >   }
> > -/* this performs a "roll", so that the element which was at index_from becomes
> > - * index_to, but the order of all other elements is preserved. */
> > -static inline int array_roll(array_t* array,int index_to,int index_from,int count)
> 
> If I understand the intent from just the comment, the old code would take a
> directory listing of six files:
> 
> ABCDEF
> 
> and on the request to delete file C, would produce:
> 
> ABFDE
> 
> by moving just F, instead of all of DEF.

I think what the old code did was actually moving C, not F, so you get:

ABDEFC

And then you reduce the array size by one so that C isn't visible any
more. My code preserves this behaviour, except that the invisible final
element is a copy of F instead C now.

Kevin