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 <address@hidden>
> >>> ---
> >>>  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.
>