[PATCH v1] migration: fix RAMBlock add NULL check

[PATCH v1] migration: fix RAMBlock add NULL check

[Dmitry Frolov]

qemu_ram_block_from_host() may return NULL, which will be dereferenced w/o
check. Usualy return value is checked for this function.
Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: c7c0e72408df5e7821c0e995122fb2fe0ac001f1 ("migration/ram: Handle RAM block resizes during precopy")
Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 migration/ram.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index e4bfd39f08..bd4b7574e1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4281,6 +4281,11 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
     RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
     Error *err = NULL;
 
+    if (!rb) {
+        error_report("RAM block not found");
+        return;
+    }
+
     if (migrate_ram_is_ignored(rb)) {
         return;
     }
-- 
2.34.1

Re: [PATCH v1] migration: fix RAMBlock add NULL check

[Fabiano Rosas]

Dmitry Frolov <frolov@swemel.ru> writes:

> qemu_ram_block_from_host() may return NULL, which will be dereferenced w/o
> check. Usualy return value is checked for this function.
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: c7c0e72408df5e7821c0e995122fb2fe0ac001f1 ("migration/ram: Handle RAM block resizes during precopy")
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> ---
>  migration/ram.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index e4bfd39f08..bd4b7574e1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4281,6 +4281,11 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>      RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>      Error *err = NULL;
>  
> +    if (!rb) {
> +        error_report("RAM block not found");
> +        return;
> +    }
> +
>      if (migrate_ram_is_ignored(rb)) {
>          return;
>      }

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v1] migration: fix RAMBlock add NULL check

[Peter Xu]

On Tue, Oct 10, 2023 at 01:48:53PM +0300, Dmitry Frolov wrote:
> qemu_ram_block_from_host() may return NULL, which will be dereferenced w/o

AFAIU this path is only called from trusted sites, so I don't see why it
can be NULL?  Do you have any scenario that can trigger this?

> check. Usualy return value is checked for this function.
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: c7c0e72408df5e7821c0e995122fb2fe0ac001f1 ("migration/ram: Handle RAM block resizes during precopy")

Normally if we attach Fixes it means we want to backport it to stable.
Here I'd like to double check on above to see whether we'd need a Fixes.

> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>

The patch itself looks all fine; though if I'm going to add some print, I'd
print something more to make it at least try to be more useful (host,
old_size, new_size).  I had a feeling that we can already assert.

Thanks,

> ---
>  migration/ram.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index e4bfd39f08..bd4b7574e1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4281,6 +4281,11 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>      RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>      Error *err = NULL;
>  
> +    if (!rb) {
> +        error_report("RAM block not found");
> +        return;
> +    }
> +
>      if (migrate_ram_is_ignored(rb)) {
>          return;
>      }
> -- 
> 2.34.1
> 
> 

-- 
Peter Xu

Re: [PATCH v1] migration: fix RAMBlock add NULL check

[Juan Quintela]

Dmitry Frolov <frolov@swemel.ru> wrote:
> qemu_ram_block_from_host() may return NULL, which will be dereferenced w/o
> check. Usualy return value is checked for this function.
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: c7c0e72408df5e7821c0e995122fb2fe0ac001f1 ("migration/ram: Handle RAM block resizes during precopy")
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [PATCH v1] migration: fix RAMBlock add NULL check

[Дмитрий Фролов]

On 10.10.2023 22:23, Peter Xu wrote:
> On Tue, Oct 10, 2023 at 01:48:53PM +0300, Dmitry Frolov wrote:
>> qemu_ram_block_from_host() may return NULL, which will be dereferenced w/o
> AFAIU this path is only called from trusted sites, so I don't see why it
> can be NULL?  Do you have any scenario that can trigger this?
No, actually no exact case. This was found by static analyzer.
I am also not sure, if NULL is possible here.
>
>> check. Usualy return value is checked for this function.
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Fixes: c7c0e72408df5e7821c0e995122fb2fe0ac001f1 ("migration/ram: Handle RAM block resizes during precopy")
> Normally if we attach Fixes it means we want to backport it to stable.
> Here I'd like to double check on above to see whether we'd need a Fixes.
>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> The patch itself looks all fine; though if I'm going to add some print, I'd
> print something more to make it at least try to be more useful (host,
> old_size, new_size).  I had a feeling that we can already assert.
I do not insist on accepting this patch - it is more like RFC.
Also, i can add more verbose message and assert, if necessary.
> Thanks,
>
>> ---
>>   migration/ram.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index e4bfd39f08..bd4b7574e1 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -4281,6 +4281,11 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier *n, void *host,
>>       RAMBlock *rb = qemu_ram_block_from_host(host, false, &offset);
>>       Error *err = NULL;
>>   
>> +    if (!rb) {
>> +        error_report("RAM block not found");
>> +        return;
>> +    }
>> +
>>       if (migrate_ram_is_ignored(rb)) {
>>           return;
>>       }
>> -- 
>> 2.34.1
>>
>>

Re: [PATCH v1] migration: fix RAMBlock add NULL check

[Peter Xu]

On Wed, Oct 11, 2023 at 04:20:42PM +0300, Дмитрий Фролов wrote:
> I do not insist on accepting this patch - it is more like RFC.
> Also, i can add more verbose message and assert, if necessary.

That's totally fine. It's just that then we should drop the Fixes line
above because it doesn't need to be backported to stable.

Also feel free to add more verbose print message or assert if you're
posting a new version.

Thanks,

-- 
Peter Xu

Re: [PATCH v1] migration: fix RAMBlock add NULL check

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Oct 11, 2023 at 04:20:42PM +0300, Дмитрий Фролов wrote:
>> I do not insist on accepting this patch - it is more like RFC.
>> Also, i can add more verbose message and assert, if necessary.
>
> That's totally fine. It's just that then we should drop the Fixes line
> above because it doesn't need to be backported to stable.
>
> Also feel free to add more verbose print message or assert if you're
> posting a new version.

I queued it as it is.

I can drop the Fixes if required.

Later, Juan.

Re: [PATCH v1] migration: fix RAMBlock add NULL check

[Дмитрий Фролов]

On 11.10.2023 17:33, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
>> On Wed, Oct 11, 2023 at 04:20:42PM +0300, Дмитрий Фролов wrote:
>>> I do not insist on accepting this patch - it is more like RFC.
>>> Also, i can add more verbose message and assert, if necessary.
>> That's totally fine. It's just that then we should drop the Fixes line
>> above because it doesn't need to be backported to stable.
>>
>> Also feel free to add more verbose print message or assert if you're
>> posting a new version.
> I queued it as it is.
Many thanks!
>
> I can drop the Fixes if required.
Would be nice. Thanks a lot!
> Later, Juan.
>
Regards, Dmitry.