diff mbox series

[v10,01/12] parallels: Out of image offset in BAT leads to image inflation

Message ID 20230203091854.2221397-2-alexander.ivanov@virtuozzo.com
State New
Headers show
Series parallels: Refactor the code of images checks and fix a bug | expand

Commit Message

Alexander Ivanov Feb. 3, 2023, 9:18 a.m. UTC 
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 <alexander.ivanov@virtuozzo.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
---
 block/parallels.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Vladimir Sementsov-Ogievskiy Feb. 14, 2023, 5:44 p.m. UTC | #1 
On 03.02.23 12:18, Alexander Ivanov wrote:
> 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 <alexander.ivanov@virtuozzo.com>
> Reviewed-by: Denis V. Lunev <den@openvz.org>
> ---
>   block/parallels.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index bbea2f2221..4af68adc61 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>       BDRVParallelsState *s = bs->opaque;
>       ParallelsHeader ph;
>       int ret, size, i;
> +    int64_t file_size;
>       QemuOpts *opts = NULL;
>       Error *local_err = NULL;
>       char *buf;
> @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>           return ret;
>       }
>   
> +    file_size = bdrv_getlength(bs->file->bs);
> +    if (file_size < 0) {
> +        return -EINVAL;
> +    }
> +    file_size >>= BDRV_SECTOR_BITS;

if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not correct, DIV_ROUND_UP would be better

> +
>       ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
>       if (ret < 0) {
>           goto fail;
> @@ -805,6 +812,16 @@ 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_size) {
> +            if (flags & BDRV_O_CHECK) {
> +                continue;
> +            }
> +            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
> +                       "is larger than file size (%" PRIi64 ")",
> +                       off, i, file_size);

offsets in sectors rather than in bytes may be a bit misleading

> +            ret = -EINVAL;
> +            goto fail;
> +        }
>           if (off >= s->data_end) {
>               s->data_end = off + s->tracks;
>           }
Denis V. Lunev Feb. 15, 2023, 11:29 a.m. UTC | #2 
On 2/14/23 18:44, Vladimir Sementsov-Ogievskiy wrote:
> On 03.02.23 12:18, Alexander Ivanov wrote:
>> 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 <alexander.ivanov@virtuozzo.com>
>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   block/parallels.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/block/parallels.c b/block/parallels.c
>> index bbea2f2221..4af68adc61 100644
>> --- a/block/parallels.c
>> +++ b/block/parallels.c
>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, 
>> QDict *options, int flags,
>>       BDRVParallelsState *s = bs->opaque;
>>       ParallelsHeader ph;
>>       int ret, size, i;
>> +    int64_t file_size;
>>       QemuOpts *opts = NULL;
>>       Error *local_err = NULL;
>>       char *buf;
>> @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, 
>> QDict *options, int flags,
>>           return ret;
>>       }
>>   +    file_size = bdrv_getlength(bs->file->bs);
>> +    if (file_size < 0) {
>> +        return -EINVAL;
>> +    }
>> +    file_size >>= BDRV_SECTOR_BITS;
>
> if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not 
> correct, DIV_ROUND_UP would be better
>
I would say that if file length is not aligned to block size - this is a
point to mark such file as broken and call check immediately.


>> +
>>       ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
>>       if (ret < 0) {
>>           goto fail;
>> @@ -805,6 +812,16 @@ 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_size) {
>> +            if (flags & BDRV_O_CHECK) {
>> +                continue;
>> +            }
>> +            error_setg(errp, "parallels: Offset %" PRIi64 " in 
>> BAT[%d] entry "
>> +                       "is larger than file size (%" PRIi64 ")",
>> +                       off, i, file_size);
>
> offsets in sectors rather than in bytes may be a bit misleading
>

agree. Should be easy

>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>>           if (off >= s->data_end) {
>>               s->data_end = off + s->tracks;
>>           }
>
Vladimir Sementsov-Ogievskiy Feb. 15, 2023, 11:42 a.m. UTC | #3 
On 15.02.23 14:29, Denis V. Lunev wrote:
> On 2/14/23 18:44, Vladimir Sementsov-Ogievskiy wrote:
>> On 03.02.23 12:18, Alexander Ivanov wrote:
>>> 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 <alexander.ivanov@virtuozzo.com>
>>> Reviewed-by: Denis V. Lunev <den@openvz.org>
>>> ---
>>>   block/parallels.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/block/parallels.c b/block/parallels.c
>>> index bbea2f2221..4af68adc61 100644
>>> --- a/block/parallels.c
>>> +++ b/block/parallels.c
>>> @@ -732,6 +732,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>       BDRVParallelsState *s = bs->opaque;
>>>       ParallelsHeader ph;
>>>       int ret, size, i;
>>> +    int64_t file_size;
>>>       QemuOpts *opts = NULL;
>>>       Error *local_err = NULL;
>>>       char *buf;
>>> @@ -741,6 +742,12 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>>>           return ret;
>>>       }
>>>   +    file_size = bdrv_getlength(bs->file->bs);
>>> +    if (file_size < 0) {
>>> +        return -EINVAL;
>>> +    }
>>> +    file_size >>= BDRV_SECTOR_BITS;
>>
>> if file size somehow not aligned to BDRV_SECTOR_SIZE, that's not correct, DIV_ROUND_UP would be better
>>
> I would say that if file length is not aligned to block size - this is a
> point to mark such file as broken and call check immediately.

I'm not sure about parallels driver, but qcow2 happily produces unaligned to sector files, when used with file opened in cached mode.

But I remembered now, that bdrv_getlength actually can't return unaligned length, as bdrv_co_getlength() is just a wrapper on bdrv_co_nb_sectors(). Then, seems better directly use bdrv_nb_sectors().
diff mbox series

Patch

diff --git a/block/parallels.c b/block/parallels.c
index bbea2f2221..4af68adc61 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -732,6 +732,7 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     BDRVParallelsState *s = bs->opaque;
     ParallelsHeader ph;
     int ret, size, i;
+    int64_t file_size;
     QemuOpts *opts = NULL;
     Error *local_err = NULL;
     char *buf;
@@ -741,6 +742,12 @@  static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         return ret;
     }
 
+    file_size = bdrv_getlength(bs->file->bs);
+    if (file_size < 0) {
+        return -EINVAL;
+    }
+    file_size >>= BDRV_SECTOR_BITS;
+
     ret = bdrv_pread(bs->file, 0, sizeof(ph), &ph, 0);
     if (ret < 0) {
         goto fail;
@@ -805,6 +812,16 @@  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_size) {
+            if (flags & BDRV_O_CHECK) {
+                continue;
+            }
+            error_setg(errp, "parallels: Offset %" PRIi64 " in BAT[%d] entry "
+                       "is larger than file size (%" PRIi64 ")",
+                       off, i, file_size);
+            ret = -EINVAL;
+            goto fail;
+        }
         if (off >= s->data_end) {
             s->data_end = off + s->tracks;
         }