* [Qemu-devel] [PATCH v2 0/2] misc fixes found by static analyzer @ 2018-06-13 8:19 Dima Stepanov 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor Dima Stepanov 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference Dima Stepanov 0 siblings, 2 replies; 11+ messages in thread From: Dima Stepanov @ 2018-06-13 8:19 UTC (permalink / raw) To: qemu-devel; +Cc: wrfsh During the development process we used scan-build as static analyzer to check the changes. There are some issues found. The patch set below is to resolve issues found. Changes v2: - remove one patch, since it was resolved by: 7eb24009 Dima Stepanov (2): memfd: fix possible usage of the uninitialized file descriptor memory: fix possible NULL pointer dereference memory.c | 2 +- util/memfd.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor 2018-06-13 8:19 [Qemu-devel] [PATCH v2 0/2] misc fixes found by static analyzer Dima Stepanov @ 2018-06-13 8:19 ` Dima Stepanov 2018-06-13 10:00 ` Marc-André Lureau 2018-08-31 17:16 ` Thomas Huth 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference Dima Stepanov 1 sibling, 2 replies; 11+ messages in thread From: Dima Stepanov @ 2018-06-13 8:19 UTC (permalink / raw) To: qemu-devel; +Cc: wrfsh The qemu_memfd_alloc_check() routine allocates the fd variable on stack. This variable is initialized inside the qemu_memfd_alloc() function. There are several cases when *fd will be left unintialized which can lead to the unexpected close() in the qemu_memfd_free() call. Set file descriptor to -1 before calling the qemu_memfd_alloc routine. Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> --- util/memfd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/util/memfd.c b/util/memfd.c index d248a53..6287946 100644 --- a/util/memfd.c +++ b/util/memfd.c @@ -187,6 +187,7 @@ bool qemu_memfd_alloc_check(void) int fd; void *ptr; + fd = -1; ptr = qemu_memfd_alloc("test", 4096, 0, &fd, NULL); memfd_check = ptr ? MEMFD_OK : MEMFD_KO; qemu_memfd_free(ptr, 4096, fd); -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor Dima Stepanov @ 2018-06-13 10:00 ` Marc-André Lureau 2018-08-31 17:16 ` Thomas Huth 1 sibling, 0 replies; 11+ messages in thread From: Marc-André Lureau @ 2018-06-13 10:00 UTC (permalink / raw) To: Dima Stepanov; +Cc: QEMU, wrfsh On Wed, Jun 13, 2018 at 10:19 AM, Dima Stepanov <dimastep@yandex-team.ru> wrote: > The qemu_memfd_alloc_check() routine allocates the fd variable on stack. > This variable is initialized inside the qemu_memfd_alloc() function. > There are several cases when *fd will be left unintialized which can > lead to the unexpected close() in the qemu_memfd_free() call. > > Set file descriptor to -1 before calling the qemu_memfd_alloc routine. > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > util/memfd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/util/memfd.c b/util/memfd.c > index d248a53..6287946 100644 > --- a/util/memfd.c > +++ b/util/memfd.c > @@ -187,6 +187,7 @@ bool qemu_memfd_alloc_check(void) > int fd; > void *ptr; > > + fd = -1; > ptr = qemu_memfd_alloc("test", 4096, 0, &fd, NULL); > memfd_check = ptr ? MEMFD_OK : MEMFD_KO; > qemu_memfd_free(ptr, 4096, fd); > -- > 2.7.4 > > -- Marc-André Lureau ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor Dima Stepanov 2018-06-13 10:00 ` Marc-André Lureau @ 2018-08-31 17:16 ` Thomas Huth 1 sibling, 0 replies; 11+ messages in thread From: Thomas Huth @ 2018-08-31 17:16 UTC (permalink / raw) To: Dima Stepanov, qemu-devel Cc: wrfsh, Paolo Bonzini, Marc-André Lureau, Markus Armbruster On 2018-06-13 10:19, Dima Stepanov wrote: > The qemu_memfd_alloc_check() routine allocates the fd variable on stack. > This variable is initialized inside the qemu_memfd_alloc() function. > There are several cases when *fd will be left unintialized which can > lead to the unexpected close() in the qemu_memfd_free() call. > > Set file descriptor to -1 before calling the qemu_memfd_alloc routine. > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > --- > util/memfd.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/util/memfd.c b/util/memfd.c > index d248a53..6287946 100644 > --- a/util/memfd.c > +++ b/util/memfd.c > @@ -187,6 +187,7 @@ bool qemu_memfd_alloc_check(void) > int fd; (in case you respin: You could also write "int fd = -1;" in above line) > void *ptr; > > + fd = -1; > ptr = qemu_memfd_alloc("test", 4096, 0, &fd, NULL); > memfd_check = ptr ? MEMFD_OK : MEMFD_KO; > qemu_memfd_free(ptr, 4096, fd); > Reviewed-by: Thomas Huth <thuth@redhat.com> (FWIW: GCC complains with -O3 about this uninitialized variable, too, so the build breaks with -Werror here - just noticed this today) ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference 2018-06-13 8:19 [Qemu-devel] [PATCH v2 0/2] misc fixes found by static analyzer Dima Stepanov 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor Dima Stepanov @ 2018-06-13 8:19 ` Dima Stepanov 2018-06-19 14:12 ` Dima Stepanov 1 sibling, 1 reply; 11+ messages in thread From: Dima Stepanov @ 2018-06-13 8:19 UTC (permalink / raw) To: qemu-devel; +Cc: wrfsh In the memory_region_do_invalidate_mmio_ptr() routine the section variable is intialized by the memory_region_find() call. The section.mr field can be set to NULL. Add the check for NULL before trying to drop a section. Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> --- memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/memory.c b/memory.c index 3212acc..bb45248 100644 --- a/memory.c +++ b/memory.c @@ -2712,7 +2712,7 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, /* Reset dirty so this doesn't happen later. */ cpu_physical_memory_test_and_clear_dirty(offset, size, 1); - if (section.mr != mr) { + if (section.mr && (section.mr != mr)) { /* memory_region_find add a ref on section.mr */ memory_region_unref(section.mr); if (MMIO_INTERFACE(section.mr->owner)) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference Dima Stepanov @ 2018-06-19 14:12 ` Dima Stepanov 2018-07-11 8:34 ` Dima Stepanov 0 siblings, 1 reply; 11+ messages in thread From: Dima Stepanov @ 2018-06-19 14:12 UTC (permalink / raw) To: qemu-devel; +Cc: wrfsh, pbonzini Ping. I believe i forgot to add the maintainer to CC. + pbonzini@redhat.com Regards, Dima. On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote: > In the memory_region_do_invalidate_mmio_ptr() routine the section > variable is intialized by the memory_region_find() call. The section.mr > field can be set to NULL. > > Add the check for NULL before trying to drop a section. > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > --- > memory.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/memory.c b/memory.c > index 3212acc..bb45248 100644 > --- a/memory.c > +++ b/memory.c > @@ -2712,7 +2712,7 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, > /* Reset dirty so this doesn't happen later. */ > cpu_physical_memory_test_and_clear_dirty(offset, size, 1); > > - if (section.mr != mr) { > + if (section.mr && (section.mr != mr)) { > /* memory_region_find add a ref on section.mr */ > memory_region_unref(section.mr); > if (MMIO_INTERFACE(section.mr->owner)) { > -- > 2.7.4 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference 2018-06-19 14:12 ` Dima Stepanov @ 2018-07-11 8:34 ` Dima Stepanov 2018-07-11 13:47 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 11+ messages in thread From: Dima Stepanov @ 2018-07-11 8:34 UTC (permalink / raw) To: qemu-devel; +Cc: wrfsh, pbonzini Gentle ping. CCing Paolo Bonzini. Regards, Dima. On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote: > Ping. > > Regards, Dima. > > On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote: > > In the memory_region_do_invalidate_mmio_ptr() routine the section > > variable is intialized by the memory_region_find() call. The section.mr > > field can be set to NULL. > > > > Add the check for NULL before trying to drop a section. > > > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > > --- > > memory.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/memory.c b/memory.c > > index 3212acc..bb45248 100644 > > --- a/memory.c > > +++ b/memory.c > > @@ -2712,7 +2712,7 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, > > /* Reset dirty so this doesn't happen later. */ > > cpu_physical_memory_test_and_clear_dirty(offset, size, 1); > > > > - if (section.mr != mr) { > > + if (section.mr && (section.mr != mr)) { > > /* memory_region_find add a ref on section.mr */ > > memory_region_unref(section.mr); > > if (MMIO_INTERFACE(section.mr->owner)) { > > -- > > 2.7.4 > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference 2018-07-11 8:34 ` Dima Stepanov @ 2018-07-11 13:47 ` Philippe Mathieu-Daudé 2018-07-11 14:09 ` Peter Maydell 2018-07-11 16:01 ` Dima Stepanov 0 siblings, 2 replies; 11+ messages in thread From: Philippe Mathieu-Daudé @ 2018-07-11 13:47 UTC (permalink / raw) To: Dima Stepanov, qemu-devel; +Cc: pbonzini, wrfsh [-- Attachment #1: Type: text/plain, Size: 1431 bytes --] Hi Dima, On 07/11/2018 05:34 AM, Dima Stepanov wrote: > Gentle ping. CCing Paolo Bonzini. > > Regards, Dima. > > On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote: >> Ping. >> >> Regards, Dima. >> >> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote: >>> In the memory_region_do_invalidate_mmio_ptr() routine the section >>> variable is intialized by the memory_region_find() call. The section.mr >>> field can be set to NULL. >>> >>> Add the check for NULL before trying to drop a section. >>> >>> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> >>> --- >>> memory.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/memory.c b/memory.c >>> index 3212acc..bb45248 100644 >>> --- a/memory.c >>> +++ b/memory.c >>> @@ -2712,7 +2712,7 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, >>> /* Reset dirty so this doesn't happen later. */ >>> cpu_physical_memory_test_and_clear_dirty(offset, size, 1); >>> >>> - if (section.mr != mr) { >>> + if (section.mr && (section.mr != mr)) { section.mr can't be NULL here. You can give the static analyzer a hint using "assert(section.mr);" >>> /* memory_region_find add a ref on section.mr */ >>> memory_region_unref(section.mr); >>> if (MMIO_INTERFACE(section.mr->owner)) { >>> -- >>> 2.7.4 >>> Regards, Phil. [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference 2018-07-11 13:47 ` Philippe Mathieu-Daudé @ 2018-07-11 14:09 ` Peter Maydell 2018-07-11 16:03 ` Dima Stepanov 2018-07-11 16:01 ` Dima Stepanov 1 sibling, 1 reply; 11+ messages in thread From: Peter Maydell @ 2018-07-11 14:09 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Dima Stepanov, QEMU Developers, Paolo Bonzini, wrfsh On 11 July 2018 at 14:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Dima, > > On 07/11/2018 05:34 AM, Dima Stepanov wrote: >> Gentle ping. CCing Paolo Bonzini. >> >> Regards, Dima. >> >> On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote: >>> Ping. >>> >>> Regards, Dima. >>> >>> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote: >>>> In the memory_region_do_invalidate_mmio_ptr() routine the section >>>> variable is intialized by the memory_region_find() call. The section.mr >>>> field can be set to NULL. >>>> >>>> Add the check for NULL before trying to drop a section. >>>> >>>> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> >>>> --- >>>> memory.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/memory.c b/memory.c >>>> index 3212acc..bb45248 100644 >>>> --- a/memory.c >>>> +++ b/memory.c >>>> @@ -2712,7 +2712,7 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, >>>> /* Reset dirty so this doesn't happen later. */ >>>> cpu_physical_memory_test_and_clear_dirty(offset, size, 1); >>>> >>>> - if (section.mr != mr) { >>>> + if (section.mr && (section.mr != mr)) { > > section.mr can't be NULL here. > > You can give the static analyzer a hint using "assert(section.mr);" Not in my view much point in messing with this code, though: (a) it's broken and unusable as it stands (race conditions) (b) it's obsoleted by the execute-from-mmio patchset http://patchwork.ozlabs.org/cover/942090/ and if/when that goes in it will all just get deleted. thanks -- PMM ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference 2018-07-11 14:09 ` Peter Maydell @ 2018-07-11 16:03 ` Dima Stepanov 0 siblings, 0 replies; 11+ messages in thread From: Dima Stepanov @ 2018-07-11 16:03 UTC (permalink / raw) To: Peter Maydell Cc: Philippe Mathieu-Daudé, QEMU Developers, Paolo Bonzini, wrfsh On Wed, Jul 11, 2018 at 03:09:13PM +0100, Peter Maydell wrote: > On 11 July 2018 at 14:47, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Hi Dima, > > > > On 07/11/2018 05:34 AM, Dima Stepanov wrote: > >> Gentle ping. CCing Paolo Bonzini. > >> > >> Regards, Dima. > >> > >> On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote: > >>> Ping. > >>> > >>> Regards, Dima. > >>> > >>> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote: > >>>> In the memory_region_do_invalidate_mmio_ptr() routine the section > >>>> variable is intialized by the memory_region_find() call. The section.mr > >>>> field can be set to NULL. > >>>> > >>>> Add the check for NULL before trying to drop a section. > >>>> > >>>> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > >>>> --- > >>>> memory.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/memory.c b/memory.c > >>>> index 3212acc..bb45248 100644 > >>>> --- a/memory.c > >>>> +++ b/memory.c > >>>> @@ -2712,7 +2712,7 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, > >>>> /* Reset dirty so this doesn't happen later. */ > >>>> cpu_physical_memory_test_and_clear_dirty(offset, size, 1); > >>>> > >>>> - if (section.mr != mr) { > >>>> + if (section.mr && (section.mr != mr)) { > > > > section.mr can't be NULL here. > > > > You can give the static analyzer a hint using "assert(section.mr);" > > Not in my view much point in messing with this code, though: > (a) it's broken and unusable as it stands (race conditions) > (b) it's obsoleted by the execute-from-mmio patchset > http://patchwork.ozlabs.org/cover/942090/ and if/when that > goes in it will all just get deleted. Got it. Thanks, Dima. > > thanks > -- PMM ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference 2018-07-11 13:47 ` Philippe Mathieu-Daudé 2018-07-11 14:09 ` Peter Maydell @ 2018-07-11 16:01 ` Dima Stepanov 1 sibling, 0 replies; 11+ messages in thread From: Dima Stepanov @ 2018-07-11 16:01 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: qemu-devel, pbonzini, wrfsh Hi Phil, On Wed, Jul 11, 2018 at 10:47:18AM -0300, Philippe Mathieu-Daudé wrote: > Hi Dima, > > On 07/11/2018 05:34 AM, Dima Stepanov wrote: > > Gentle ping. CCing Paolo Bonzini. > > > > Regards, Dima. > > > > On Tue, Jun 19, 2018 at 05:12:16PM +0300, Dima Stepanov wrote: > >> Ping. > >> > >> Regards, Dima. > >> > >> On Wed, Jun 13, 2018 at 11:19:55AM +0300, Dima Stepanov wrote: > >>> In the memory_region_do_invalidate_mmio_ptr() routine the section > >>> variable is intialized by the memory_region_find() call. The section.mr > >>> field can be set to NULL. > >>> > >>> Add the check for NULL before trying to drop a section. > >>> > >>> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > >>> --- > >>> memory.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/memory.c b/memory.c > >>> index 3212acc..bb45248 100644 > >>> --- a/memory.c > >>> +++ b/memory.c > >>> @@ -2712,7 +2712,7 @@ static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu, > >>> /* Reset dirty so this doesn't happen later. */ > >>> cpu_physical_memory_test_and_clear_dirty(offset, size, 1); > >>> > >>> - if (section.mr != mr) { > >>> + if (section.mr && (section.mr != mr)) { > > section.mr can't be NULL here. > > You can give the static analyzer a hint using "assert(section.mr);" My point was: section = memory_region_find(mr, offset, size) -> ret = memory_region_find_rcu(mr, addr, size); -> MemoryRegionSection ret = { .mr = NULL }; ... as = memory_region_to_address_space(root); if this call returns NULL, then the ret value will be returned (with .mr == NULL) But maybe it is impossible for this case. Thanks for the reply. Regards, Dima. > > >>> /* memory_region_find add a ref on section.mr */ > >>> memory_region_unref(section.mr); > >>> if (MMIO_INTERFACE(section.mr->owner)) { > >>> -- > >>> 2.7.4 > >>> > > Regards, > > Phil. > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-08-31 17:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-13 8:19 [Qemu-devel] [PATCH v2 0/2] misc fixes found by static analyzer Dima Stepanov 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor Dima Stepanov 2018-06-13 10:00 ` Marc-André Lureau 2018-08-31 17:16 ` Thomas Huth 2018-06-13 8:19 ` [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference Dima Stepanov 2018-06-19 14:12 ` Dima Stepanov 2018-07-11 8:34 ` Dima Stepanov 2018-07-11 13:47 ` Philippe Mathieu-Daudé 2018-07-11 14:09 ` Peter Maydell 2018-07-11 16:03 ` Dima Stepanov 2018-07-11 16:01 ` Dima Stepanov
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.