[Qemu-devel] [PATCH] migration: catch unknown flags in ram_load

[Qemu-devel] [PATCH] migration: catch unknown flags in ram_load

[Peter Lieven]

if a saved vm has unknown flags in the memory data qemu
currently simply ignores this flag and continues which
yields in an unpredictable result.

this patch catches all unknown flags and
aborts the loading of the vm.

CC: qemu-stable@nongnu.org
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 arch_init.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 995f56d..582b716 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1084,9 +1084,7 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
 
                 total_ram_bytes -= length;
             }
-        }
-
-        if (flags & RAM_SAVE_FLAG_COMPRESS) {
+        } else if (flags & RAM_SAVE_FLAG_COMPRESS) {
             void *host;
             uint8_t ch;
 
@@ -1121,6 +1119,9 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
             }
         } else if (flags & RAM_SAVE_FLAG_HOOK) {
             ram_control_load_hook(f, flags);
+        } else if (!(flags & RAM_SAVE_FLAG_EOS)) {
+            ret = -EINVAL;
+            goto done;
         }
         error = qemu_file_get_error(f);
         if (error) {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH] migration: catch unknown flags in ram_load

[陈梁]

> if a saved vm has unknown flags in the memory data qemu
> currently simply ignores this flag and continues which
> yields in an unpredictable result.
> 
> this patch catches all unknown flags and
> aborts the loading of the vm.
> 
> CC: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> arch_init.c |    7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 

Reviewed-by: ChenLiang <chenliang0016@icloud.com>

Re: [Qemu-devel] [PATCH] migration: catch unknown flags in ram_load

[Juan Quintela]

Peter Lieven <pl@kamp.de> wrote:
> if a saved vm has unknown flags in the memory data qemu
> currently simply ignores this flag and continues which
> yields in an unpredictable result.
>
> this patch catches all unknown flags and
> aborts the loading of the vm.
>
> CC: qemu-stable@nongnu.org
> Signed-off-by: Peter Lieven <pl@kamp.de>

.....

Once here, shouldn't be better to do this as:

change do {} while ()   for while (true) {}

>  
> @@ -1121,6 +1119,9 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>              }
>          } else if (flags & RAM_SAVE_FLAG_HOOK) {
>              ram_control_load_hook(f, flags);
> +        } else if (!(flags & RAM_SAVE_FLAG_EOS)) {
> +            ret = -EINVAL;
> +            goto done;
>          }
>          error = qemu_file_get_error(f);
>          if (error) {


        } else if (flags & RAM_SAVE_FLAG_HOOK) {
            ram_control_load_hook(f, flags);
+       } else if (flags & RAM_SAVE_FLAG_EOS) {
+           break;
+       } else {
+           ret = -EINVAL;
+           goto done;
        }
          error = qemu_file_get_error(f);
          if (error) {
        }


This way, we are checking RAM_SAVE_FLAG_EOS the same way than any other
flag?  And we don't have to duplicate the FLAG_NAME?

Unrelated to this patch, all the flags are a bitmap, but really, the
ones that can be together are RAM_SAVE_FLAG_CONTINUE and the rest, all
the others need to be alone.  I am telling this because we have used
already 8 flags, and we are using the low bits of offset to save the
flags, we have 10 flags?  Perhaps changing the last flag to mean that
the low bits pass to be a counter?

PD. No, I haven't investigated right now how RAM_SAVE_FLAG_HOOK works
with all of this.

Later, Juan.

Re: [Qemu-devel] [PATCH] migration: catch unknown flags in ram_load

[Peter Lieven]

Am 12.05.2014 12:19, schrieb Juan Quintela:
> Peter Lieven <pl@kamp.de> wrote:
>> if a saved vm has unknown flags in the memory data qemu
>> currently simply ignores this flag and continues which
>> yields in an unpredictable result.
>>
>> this patch catches all unknown flags and
>> aborts the loading of the vm.
>>
>> CC: qemu-stable@nongnu.org
>> Signed-off-by: Peter Lieven <pl@kamp.de>
> .....
>
> Once here, shouldn't be better to do this as:
>
> change do {} while ()   for while (true) {}
>
>>  
>> @@ -1121,6 +1119,9 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>              }
>>          } else if (flags & RAM_SAVE_FLAG_HOOK) {
>>              ram_control_load_hook(f, flags);
>> +        } else if (!(flags & RAM_SAVE_FLAG_EOS)) {
>> +            ret = -EINVAL;
>> +            goto done;
>>          }
>>          error = qemu_file_get_error(f);
>>          if (error) {
>
>         } else if (flags & RAM_SAVE_FLAG_HOOK) {
>             ram_control_load_hook(f, flags);
> +       } else if (flags & RAM_SAVE_FLAG_EOS) {
> +           break;
> +       } else {
> +           ret = -EINVAL;
> +           goto done;
>         }
>           error = qemu_file_get_error(f);
>           if (error) {
>         }
>
>
> This way, we are checking RAM_SAVE_FLAG_EOS the same way than any other
> flag?  And we don't have to duplicate the FLAG_NAME?
Ok, I will send a v2.

>
> Unrelated to this patch, all the flags are a bitmap, but really, the
> ones that can be together are RAM_SAVE_FLAG_CONTINUE and the rest, all
> the others need to be alone.  I am telling this because we have used
> already 8 flags, and we are using the low bits of offset to save the
> flags, we have 10 flags?  Perhaps changing the last flag to mean that
> the low bits pass to be a counter?

Some better encoding would indeed be useful. I already thought
that we might run out of flags soon. We have 11 flags I think,
but there is not much space left. Reserving the last flag to indicate
that the lower 10 bits a are counter might be a good option.

Peter

>
> PD. No, I haven't investigated right now how RAM_SAVE_FLAG_HOOK works
> with all of this.
>
> Later, Juan.
>

Re: [Qemu-devel] [PATCH] migration: catch unknown flags in ram_load

[Peter Lieven]

Am 12.05.2014 12:25, schrieb Peter Lieven:
> Am 12.05.2014 12:19, schrieb Juan Quintela:
>> Peter Lieven <pl@kamp.de> wrote:
>>> if a saved vm has unknown flags in the memory data qemu
>>> currently simply ignores this flag and continues which
>>> yields in an unpredictable result.
>>>
>>> this patch catches all unknown flags and
>>> aborts the loading of the vm.
>>>
>>> CC: qemu-stable@nongnu.org
>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> .....
>>
>> Once here, shouldn't be better to do this as:
>>
>> change do {} while ()   for while (true) {}
>>
>>>  
>>> @@ -1121,6 +1119,9 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>>              }
>>>          } else if (flags & RAM_SAVE_FLAG_HOOK) {
>>>              ram_control_load_hook(f, flags);
>>> +        } else if (!(flags & RAM_SAVE_FLAG_EOS)) {
>>> +            ret = -EINVAL;
>>> +            goto done;
>>>          }
>>>          error = qemu_file_get_error(f);
>>>          if (error) {
>>         } else if (flags & RAM_SAVE_FLAG_HOOK) {
>>             ram_control_load_hook(f, flags);
>> +       } else if (flags & RAM_SAVE_FLAG_EOS) {
>> +           break;
>> +       } else {
>> +           ret = -EINVAL;
>> +           goto done;
>>         }
>>           error = qemu_file_get_error(f);
>>           if (error) {

we can also drop the error variable I think and change the loop to

while (!ret) {}

>>         }
>>
>>
>> This way, we are checking RAM_SAVE_FLAG_EOS the same way than any other
>> flag?  And we don't have to duplicate the FLAG_NAME?
> Ok, I will send a v2.
>
>> Unrelated to this patch, all the flags are a bitmap, but really, the
>> ones that can be together are RAM_SAVE_FLAG_CONTINUE and the rest, all
>> the others need to be alone.  I am telling this because we have used
>> already 8 flags, and we are using the low bits of offset to save the
>> flags, we have 10 flags?  Perhaps changing the last flag to mean that
>> the low bits pass to be a counter?
> Some better encoding would indeed be useful. I already thought
> that we might run out of flags soon. We have 11 flags I think,
> but there is not much space left. Reserving the last flag to indicate
> that the lower 10 bits a are counter might be a good option.
>
> Peter
>
>> PD. No, I haven't investigated right now how RAM_SAVE_FLAG_HOOK works
>> with all of this.
>>
>> Later, Juan.
>>