[PULL,05/17] parallels: Out of image offset in BAT leads to image inflation

Commit Message

Hanna Czenczek June 5, 2023, 3:45 p.m. UTC

From: Alexander Ivanov <[e-mail]>

data_end field in BDRVParallelsState is set to the biggest offset present
in BAT. If this offset is outside of the image, any further write will
create the cluster at this offset and/or the image will be truncated to
this offset on close. This is definitely not correct.

Raise an error in parallels_open() if data_end points outside the image
and it is not a check (let the check to repaire the image). Set data_end
to the end of the cluster with the last correct offset.

Signed-off-by: Alexander Ivanov <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Hanna Czenczek <[e-mail]>
Signed-off-by: Hanna Czenczek <[e-mail]>
---
 block/parallels.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Michael Tokarev June 7, 2023, 6:51 a.m. UTC | #1

05.06.2023 18:45, Hanna Czenczek wrote:
> From: Alexander Ivanov <[e-mail]>
> 
> data_end field in BDRVParallelsState is set to the biggest offset present
> in BAT. If this offset is outside of the image, any further write will
> create the cluster at this offset and/or the image will be truncated to
> this offset on close. This is definitely not correct.
> 
> Raise an error in parallels_open() if data_end points outside the image
> and it is not a check (let the check to repaire the image). Set data_end
> to the end of the cluster with the last correct offset.

Hi!

This, and a few other parallels changes in this series:

  parallels: Out of image offset in BAT leads to image inflation
  parallels: Fix high_off calculation in parallels_co_check()
  parallels: Fix image_end_offset and data_end after out-of-image check
  parallels: Fix statistics calculation (?)

Should these be applied to -stable too, or is it not important?

Thanks,

/mjt

Michael Tokarev June 7, 2023, 8:47 a.m. UTC | #2

07.06.2023 09:51, Michael Tokarev wrote:
> 05.06.2023 18:45, Hanna Czenczek wrote:
>> From: Alexander Ivanov <[e-mail]>
>>
>> data_end field in BDRVParallelsState is set to the biggest offset present
>> in BAT. If this offset is outside of the image, any further write will
>> create the cluster at this offset and/or the image will be truncated to
>> this offset on close. This is definitely not correct.
>>
>> Raise an error in parallels_open() if data_end points outside the image
>> and it is not a check (let the check to repaire the image). Set data_end
>> to the end of the cluster with the last correct offset.
> 
> Hi!
> 
> This, and a few other parallels changes in this series:
> 
>   parallels: Out of image offset in BAT leads to image inflation
>   parallels: Fix high_off calculation in parallels_co_check()
>   parallels: Fix image_end_offset and data_end after out-of-image check
>   parallels: Fix statistics calculation (?)

And probably also:

   parallels: Incorrect condition in out-of-image check

> Should these be applied to -stable too, or is it not important?
> 
> Thanks,
> 
> /mjt
>

Hanna Czenczek June 7, 2023, 3:14 p.m. UTC | #3

On 07.06.23 08:51, Michael Tokarev wrote:
> 05.06.2023 18:45, Hanna Czenczek wrote:
>> From: Alexander Ivanov <[e-mail]>
>>
>> data_end field in BDRVParallelsState is set to the biggest offset 
>> present
>> in BAT. If this offset is outside of the image, any further write will
>> create the cluster at this offset and/or the image will be truncated to
>> this offset on close. This is definitely not correct.
>>
>> Raise an error in parallels_open() if data_end points outside the image
>> and it is not a check (let the check to repaire the image). Set data_end
>> to the end of the cluster with the last correct offset.
>
> Hi!
>
> This, and a few other parallels changes in this series:
>
>  parallels: Out of image offset in BAT leads to image inflation
>  parallels: Fix high_off calculation in parallels_co_check()
>  parallels: Fix image_end_offset and data_end after out-of-image check
>  parallels: Fix statistics calculation (?)
>
> Should these be applied to -stable too, or is it not important?

Personally, I don’t think they need to be in stable; but I’ll leave the 
final judgment to Alexander.

Hanna

Denis V. Lunev June 9, 2023, 8:54 a.m. UTC | #4

On 6/7/23 17:14, Hanna Czenczek wrote:
> On 07.06.23 08:51, Michael Tokarev wrote:
>> 05.06.2023 18:45, Hanna Czenczek wrote:
>>> From: Alexander Ivanov <[e-mail]>
>>>
>>> data_end field in BDRVParallelsState is set to the biggest offset 
>>> present
>>> in BAT. If this offset is outside of the image, any further write will
>>> create the cluster at this offset and/or the image will be truncated to
>>> this offset on close. This is definitely not correct.
>>>
>>> Raise an error in parallels_open() if data_end points outside the image
>>> and it is not a check (let the check to repaire the image). Set 
>>> data_end
>>> to the end of the cluster with the last correct offset.
>>
>> Hi!
>>
>> This, and a few other parallels changes in this series:
>>
>>  parallels: Out of image offset in BAT leads to image inflation
>>  parallels: Fix high_off calculation in parallels_co_check()
>>  parallels: Fix image_end_offset and data_end after out-of-image check
>>  parallels: Fix statistics calculation (?)
>>
>> Should these be applied to -stable too, or is it not important?
>
> Personally, I don’t think they need to be in stable; but I’ll leave 
> the final judgment to Alexander.
>
> Hanna
>
>
I do not think that this needs to go to stable too.

Thanks you in advance,
     Den

Michael Tokarev June 9, 2023, 9:05 a.m. UTC | #5

09.06.2023 11:54, Denis V. Lunev wrote:
> On 6/7/23 17:14, Hanna Czenczek wrote:
..>>> This, and a few other parallels changes in this series:
>>> Should these be applied to -stable too, or is it not important?
>>
>> Personally, I don’t think they need to be in stable; but I’ll leave the final judgment to Alexander.
>>
> I do not think that this needs to go to stable too.

Ok, excellent, thank you!

/mjt

Patch

diff --git a/block/parallels.c b/block/parallels.c
index d8a3f13e24..7b6d770f8e 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -733,6 +733,7 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
     int ret, size, i;
+    int64_t file_nb_sectors;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -742,6 +743,11 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
+    file_nb_sectors = bdrv_nb_sectors(bs->file->bs);
+    if (file_nb_sectors < 0) {
+        return -EINVAL;
+    }
+
     ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
     if (ret < 0) {
         goto fail;
@@ -806,6 +812,17 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
 
     for (i = 0; i < s->bat_size; i++) {
         int64_t off = bat2sect(s, i);
+        if (off >= file_nb_sectors) {
+            if (flags & BDRV_O_CHECK) {
+                continue;
+            }
+            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
+                       "is larger than file size (%" PRIi64 ")",
+                       off << BDRV_SECTOR_BITS, i,
+                       file_nb_sectors << BDRV_SECTOR_BITS);
+            ret = -EINVAL;
+            goto fail;
+        }
         if (off >= s->data_end) {
             s->data_end = off + s->tracks;
         }