* [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
* [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 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 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 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
* 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 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
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.