diff mbox series

[v1] migration: fix RAMBlock add NULL check

Message ID 20231010104851.802947-1-frolov@swemel.ru
State New
Headers show
Series [v1] migration: fix RAMBlock add NULL check | expand

Commit Message

Dmitry Frolov Oct. 10, 2023, 10:48 a.m. UTC
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(+)

Comments

Fabiano Rosas Oct. 10, 2023, 1:36 p.m. UTC | #1
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>
Peter Xu Oct. 10, 2023, 7:23 p.m. UTC | #2
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
> 
>
Juan Quintela Oct. 11, 2023, 1:07 p.m. UTC | #3
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>
Dmitry Frolov Oct. 11, 2023, 1:20 p.m. UTC | #4
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
>>
>>
Peter Xu Oct. 11, 2023, 2:24 p.m. UTC | #5
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,
Juan Quintela Oct. 11, 2023, 2:33 p.m. UTC | #6
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.
Dmitry Frolov Oct. 11, 2023, 2:36 p.m. UTC | #7
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.
diff mbox series

Patch

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;
     }