When reading with `read_cluster` we get the `mapping` with
`find_mapping_for_cluster` and then we call `open_file` for this
mapping.
The issue appear when its the same file, but a second cluster that is
not immediately after it, imagine clusters `500 -> 503`, this will give
us 2 mappings one has the range `500..501` and another `503..504`, both
point to the same file, but different offsets.

When we don't open the file since the path is the same, we won't assign
`s->current_mapping` and thus accessing way out of bound of the file.

From our example above, after `open_file` (that didn't open anything) we
will get the offset into the file with
`s->cluster_size*(cluster_num-s->current_mapping->begin)`, which will
give us `0x2000 * (504-500)`, which is out of bound for this mapping and
will produce some issues.

Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
---
 block/vvfat.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index b63ac5d045..d2b879705e 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1360,15 +1360,17 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
 {
     if(!mapping)
         return -1;
-    if(!s->current_mapping ||
-            strcmp(s->current_mapping->path,mapping->path)) {
+    if (s->current_mapping != mapping) {
         /* open file */
         int fd = qemu_open_old(mapping->path,
-                               O_RDONLY | O_BINARY | O_LARGEFILE);
-        if(fd<0)
+                            O_RDONLY | O_BINARY | O_LARGEFILE);
+        if (fd < 0) {
             return -1;
+        }
         vvfat_close_current_file(s);
+
         s->current_fd = fd;
+        assert(s->current_fd);
         s->current_mapping = mapping;
     }
     return 0;
-- 
2.45.2

Am 20.07.2024 um 12:13 hat Amjad Alsharafi geschrieben:
> When reading with `read_cluster` we get the `mapping` with
> `find_mapping_for_cluster` and then we call `open_file` for this
> mapping.
> The issue appear when its the same file, but a second cluster that is
> not immediately after it, imagine clusters `500 -> 503`, this will give
> us 2 mappings one has the range `500..501` and another `503..504`, both
> point to the same file, but different offsets.
> 
> When we don't open the file since the path is the same, we won't assign
> `s->current_mapping` and thus accessing way out of bound of the file.
> 
> From our example above, after `open_file` (that didn't open anything) we
> will get the offset into the file with
> `s->cluster_size*(cluster_num-s->current_mapping->begin)`, which will
> give us `0x2000 * (504-500)`, which is out of bound for this mapping and
> will produce some issues.
> 
> Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
> ---
>  block/vvfat.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index b63ac5d045..d2b879705e 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1360,15 +1360,17 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
>  {
>      if(!mapping)
>          return -1;
> -    if(!s->current_mapping ||
> -            strcmp(s->current_mapping->path,mapping->path)) {
> +    if (s->current_mapping != mapping) {
>          /* open file */
>          int fd = qemu_open_old(mapping->path,
> -                               O_RDONLY | O_BINARY | O_LARGEFILE);
> -        if(fd<0)
> +                            O_RDONLY | O_BINARY | O_LARGEFILE);
> +        if (fd < 0) {
>              return -1;
> +        }
>          vvfat_close_current_file(s);
> +
>          s->current_fd = fd;
> +        assert(s->current_fd);
>          s->current_mapping = mapping;
>      }
>      return 0;

Now we're reopening the file even if the mapping is actually referring
to the same file that was already open. So the result should be correct,
but wasteful in what is probably a common case.

However, this version of the patch simplified things enough that I think
I finally see what we really want:

diff --git a/block/vvfat.c b/block/vvfat.c
index e3a83fbc88..8ffe8b3b9b 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1369,8 +1369,9 @@ static int open_file(BDRVVVFATState* s,mapping_t* mapping)
             return -1;
         vvfat_close_current_file(s);
         s->current_fd = fd;
-        s->current_mapping = mapping;
     }
+
+    s->current_mapping = mapping;
     return 0;
 }

That should be all that is needed, and it passes your test case.

I don't usually do this, but since time is running out for QEMU 9.1,
I'll just replace the code of this patch with the above and go ahead
with that. If you think it's wrong, let me know and we'll fix it on top
of this series.

Kevin

On Aug 5 2024, at 6:22 pm, Kevin Wolf <kwolf@redhat.com> wrote:

> Am 20.07.2024 um 12:13 hat Amjad Alsharafi geschrieben:
>> When reading with `read_cluster` we get the `mapping` with
>> `find_mapping_for_cluster` and then we call `open_file` for this
>> mapping.
>> The issue appear when its the same file, but a second cluster that is
>> not immediately after it, imagine clusters `500 -> 503`, this will give
>> us 2 mappings one has the range `500..501` and another `503..504`, both
>> point to the same file, but different offsets.
>> 
>> When we don't open the file since the path is the same, we won't assign
>> `s->current_mapping` and thus accessing way out of bound of the file.
>> 
>> From our example above, after `open_file` (that didn't open anything) we
>> will get the offset into the file with
>> `s->cluster_size*(cluster_num-s->current_mapping->begin)`, which will
>> give us `0x2000 * (504-500)`, which is out of bound for this mapping and
>> will produce some issues.
>> 
>> Signed-off-by: Amjad Alsharafi <amjadsharafi10@gmail.com>
>> ---
>>  block/vvfat.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index b63ac5d045..d2b879705e 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -1360,15 +1360,17 @@ static int open_file(BDRVVVFATState*
>> s,mapping_t* mapping)
>>  {
>>      if(!mapping)
>>          return -1;
>> -    if(!s->current_mapping ||
>> -            strcmp(s->current_mapping->path,mapping->path)) {
>> +    if (s->current_mapping != mapping) {
>>          /* open file */
>>          int fd = qemu_open_old(mapping->path,
>> -                               O_RDONLY | O_BINARY | O_LARGEFILE);
>> -        if(fd<0)
>> +                            O_RDONLY | O_BINARY | O_LARGEFILE);
>> +        if (fd < 0) {
>>              return -1;
>> +        }
>>          vvfat_close_current_file(s);
>> +
>>          s->current_fd = fd;
>> +        assert(s->current_fd);
>>          s->current_mapping = mapping;
>>      }
>>      return 0;
> 
> Now we're reopening the file even if the mapping is actually referring
> to the same file that was already open. So the result should be correct,
> but wasteful in what is probably a common case.
> 
> However, this version of the patch simplified things enough that I think
> I finally see what we really want:
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index e3a83fbc88..8ffe8b3b9b 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1369,8 +1369,9 @@ static int open_file(BDRVVVFATState*
> s,mapping_t* mapping)
>             return -1;
>         vvfat_close_current_file(s);
>         s->current_fd = fd;
> -        s->current_mapping = mapping;
>     }
> +
> +    s->current_mapping = mapping;
>     return 0;
> }

Ah, that indeed fixes it with such a simple change haha.

> 
> That should be all that is needed, and it passes your test case.
> 
> I don't usually do this, but since time is running out for QEMU 9.1,
> I'll just replace the code of this patch with the above and go ahead
> with that. If you think it's wrong, let me know and we'll fix it on top
> of this series.
> 
> Kevin
> 
> 

Thanks for that, and for publishing this. Looks good.

Amjad