[PATCH 0/2] Fix some problems with vvfat in R/W mode

[PATCH 0/2] Fix some problems with vvfat in R/W mode

[Hervé Poussineau]

Hi,

When testing vvfat in read-write mode, I came across some blocking
problems when using Windows guests.
This patchset is not here to fix all problems of vvfat, but only the
main ones I encountered.

First patch allows setting/resetting the 'volume dirty' flag on
boosector, and the second one allows creating file names with spaces.

Hervé

Hervé Poussineau (2):
  vvfat: allow some writes to bootsector
  vvfat: allow spaces in file names

 block/vvfat.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

-- 
2.36.2

[PATCH 1/2] vvfat: allow some writes to bootsector

[Hervé Poussineau]

'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
Allow writes to bootsector which only changes the 'reserved1' field.

This fixes I/O errors on Windows guests.

Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 block/vvfat.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index d6dd919683d..35057a51c67 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2993,11 +2993,27 @@ DLOG(checkpoint());
 
     vvfat_close_current_file(s);
 
+    if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
+        /*
+         * Write on bootsector. Allow only changing the reserved1 field,
+         * used to mark volume dirtiness
+         */
+        const unsigned char *initial = s->first_sectors
+                                       + s->offset_to_bootsector * 0x200;
+        for (i = 0; i < 0x200; i++) {
+            if (i != offsetof(bootsector_t, u.fat16.reserved1) &&
+                initial[i] != buf[i]) {
+                fprintf(stderr, "Tried to write to protected bootsector\n");
+                return -1;
+            }
+        }
+        return 0;
+    }
+
     /*
      * Some sanity checks:
      * - do not allow writing to the boot sector
      */
-
     if (sector_num < s->offset_to_fat)
         return -1;
 
-- 
2.36.2

[PATCH 2/2] vvfat: allow spaces in file names

[Hervé Poussineau]

In R/W mode, files with spaces were never created on host side.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1176
Fixes: c79e243ed67683d6d06692bd7040f7394da178b0
Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 block/vvfat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 35057a51c67..9d877028573 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -499,7 +499,7 @@ static bool valid_filename(const unsigned char *name)
               (c >= 'A' && c <= 'Z') ||
               (c >= 'a' && c <= 'z') ||
               c > 127 ||
-              strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
+              strchr(" $%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
         {
             return false;
         }
-- 
2.36.2

Re: [PATCH 2/2] vvfat: allow spaces in file names

[Philippe Mathieu-Daudé via]

On 3/9/22 18:23, Hervé Poussineau wrote:
> In R/W mode, files with spaces were never created on host side.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1176
> Fixes: c79e243ed67683d6d06692bd7040f7394da178b0
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>   block/vvfat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 35057a51c67..9d877028573 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -499,7 +499,7 @@ static bool valid_filename(const unsigned char *name)
>                 (c >= 'A' && c <= 'Z') ||
>                 (c >= 'a' && c <= 'z') ||
>                 c > 127 ||
> -              strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
> +              strchr(" $%'-_@~`!(){}^#&.+,;=[]", c) != NULL))
>           {
>               return false;
>           }

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 1/2] vvfat: allow some writes to bootsector

[Kevin Wolf]

Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
> 'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
> Allow writes to bootsector which only changes the 'reserved1' field.
> 
> This fixes I/O errors on Windows guests.
> 
> Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  block/vvfat.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index d6dd919683d..35057a51c67 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -2993,11 +2993,27 @@ DLOG(checkpoint());
>  
>      vvfat_close_current_file(s);
>  
> +    if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
> +        /*
> +         * Write on bootsector. Allow only changing the reserved1 field,
> +         * used to mark volume dirtiness
> +         */
> +        const unsigned char *initial = s->first_sectors
> +                                       + s->offset_to_bootsector * 0x200;
> +        for (i = 0; i < 0x200; i++) {
> +            if (i != offsetof(bootsector_t, u.fat16.reserved1) &&

I think you need to check the FAT version (s->fat_type) before accessing
u.fat16. For FAT32, the "reserved" field is at a different offset (but
seems to have the same meaning).

> +                initial[i] != buf[i]) {
> +                fprintf(stderr, "Tried to write to protected bootsector\n");
> +                return -1;
> +            }
> +        }
> +        return 0;
> +    }

Should we update s->first_sectors with the new value so that the guest
would actually read back what it wrote instead of having the change
disappear magically?

>      /*
>       * Some sanity checks:
>       * - do not allow writing to the boot sector
>       */
> -
>      if (sector_num < s->offset_to_fat)
>          return -1;

Kevin

Re: [PATCH 2/2] vvfat: allow spaces in file names

[Kevin Wolf]

Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
> In R/W mode, files with spaces were never created on host side.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1176
> Fixes: c79e243ed67683d6d06692bd7040f7394da178b0
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [PATCH 1/2] vvfat: allow some writes to bootsector

[Hervé Poussineau]

Le 29/09/2022 à 16:10, Kevin Wolf a écrit :
> Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
>> 'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
>> Allow writes to bootsector which only changes the 'reserved1' field.
>>
>> This fixes I/O errors on Windows guests.
>>
>> Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>   block/vvfat.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index d6dd919683d..35057a51c67 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -2993,11 +2993,27 @@ DLOG(checkpoint());
>>   
>>       vvfat_close_current_file(s);
>>   
>> +    if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
>> +        /*
>> +         * Write on bootsector. Allow only changing the reserved1 field,
>> +         * used to mark volume dirtiness
>> +         */
>> +        const unsigned char *initial = s->first_sectors
>> +                                       + s->offset_to_bootsector * 0x200;
>> +        for (i = 0; i < 0x200; i++) {
>> +            if (i != offsetof(bootsector_t, u.fat16.reserved1) &&
> 
> I think you need to check the FAT version (s->fat_type) before accessing
> u.fat16. For FAT32, the "reserved" field is at a different offset (but
> seems to have the same meaning).

I didn't do this, because only fat16 part of bootsector is ever used.
In init_directories(), only fat16 part is initialized, with the comment:
	/* LATER TODO: if FAT32, this is wrong */
I wanted to be consistent between init_directories() and the check.

> 
>> +                initial[i] != buf[i]) {
>> +                fprintf(stderr, "Tried to write to protected bootsector\n");
>> +                return -1;
>> +            }
>> +        }
>> +        return 0;
>> +    }
> 
> Should we update s->first_sectors with the new value so that the guest
> would actually read back what it wrote instead of having the change
> disappear magically?

Windows guests don't seem to care if the written value disappears. They only want the write to succeed.

> 
>>       /*
>>        * Some sanity checks:
>>        * - do not allow writing to the boot sector
>>        */
>> -
>>       if (sector_num < s->offset_to_fat)
>>           return -1;
> 
> Kevin
> 

Re: [PATCH 1/2] vvfat: allow some writes to bootsector

[Kevin Wolf]

Am 29.09.2022 um 21:53 hat Hervé Poussineau geschrieben:
> Le 29/09/2022 à 16:10, Kevin Wolf a écrit :
> > Am 03.09.2022 um 18:23 hat Hervé Poussineau geschrieben:
> > > 'reserved1' field in bootsector is used to mark volume dirty, or need to verify.
> > > Allow writes to bootsector which only changes the 'reserved1' field.
> > > 
> > > This fixes I/O errors on Windows guests.
> > > 
> > > Resolves: https://bugs.launchpad.net/qemu/+bug/1889421
> > > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> > > ---
> > >   block/vvfat.c | 18 +++++++++++++++++-
> > >   1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block/vvfat.c b/block/vvfat.c
> > > index d6dd919683d..35057a51c67 100644
> > > --- a/block/vvfat.c
> > > +++ b/block/vvfat.c
> > > @@ -2993,11 +2993,27 @@ DLOG(checkpoint());
> > >       vvfat_close_current_file(s);
> > > +    if (sector_num == s->offset_to_bootsector && nb_sectors == 1) {
> > > +        /*
> > > +         * Write on bootsector. Allow only changing the reserved1 field,
> > > +         * used to mark volume dirtiness
> > > +         */
> > > +        const unsigned char *initial = s->first_sectors
> > > +                                       + s->offset_to_bootsector * 0x200;
> > > +        for (i = 0; i < 0x200; i++) {
> > > +            if (i != offsetof(bootsector_t, u.fat16.reserved1) &&
> > 
> > I think you need to check the FAT version (s->fat_type) before accessing
> > u.fat16. For FAT32, the "reserved" field is at a different offset (but
> > seems to have the same meaning).
> 
> I didn't do this, because only fat16 part of bootsector is ever used.
> In init_directories(), only fat16 part is initialized, with the comment:
> 	/* LATER TODO: if FAT32, this is wrong */
> I wanted to be consistent between init_directories() and the check.

Oh, indeed. I guess this means FAT32 is completely broken... Fair
enough, though maybe we could add a similar comment here, then.

> > > +                initial[i] != buf[i]) {
> > > +                fprintf(stderr, "Tried to write to protected bootsector\n");
> > > +                return -1;
> > > +            }
> > > +        }
> > > +        return 0;
> > > +    }
> > 
> > Should we update s->first_sectors with the new value so that the guest
> > would actually read back what it wrote instead of having the change
> > disappear magically?
> 
> Windows guests don't seem to care if the written value disappears.
> They only want the write to succeed.

But it would be arguably more correct, wouldn't it? Some other OS might
care.

Kevin