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

["Hervé Poussineau" <hpoussin@reactos.org>]

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
>