Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference
On Wed, Jul 11, 2018 at 03:09:13PM +0100, Peter Maydell wrote:
> On 11 July 2018 at 14:47, 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
> ---
> 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
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
> >>> ---
> >>> 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.
>
Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference
On 11 July 2018 at 14:47, 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
---
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
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
>>> ---
>>> 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.
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 2/2] memory: fix possible NULL pointer dereference
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
> > ---
> > 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
Ping. I believe i forgot to add the maintainer to CC.
+ pbonz...@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
> ---
> 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
>
>
