[Qemu-devel] [PATCH v2 0/2] misc fixes found by static analyzer

[Qemu-devel] [PATCH v2 0/2] misc fixes found by static analyzer

[Dima Stepanov]

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

[Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor

[Dima Stepanov]

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

Re: [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor

[Marc-André Lureau]

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

Re: [Qemu-devel] [PATCH v2 1/2] memfd: fix possible usage of the uninitialized file descriptor

[Thomas Huth]

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)

[Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

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

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

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

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

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

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Philippe Mathieu-Daudé]

[-- 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 --]

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

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

Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference

[Dima Stepanov]

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