| Message ID | 20231010104851.802947-1-frolov@swemel.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] migration: fix RAMBlock add NULL check | expand |
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>
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 > >
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>
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 >> >>
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 <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.
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 --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; }
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(+)