[Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Olaf Hering]

The codepaths behind qemu_ram_ptr_length can return NULL.
Avoid crashing the device-model in such case, just move on.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
This happens if calling xendevicemodel_create_ioreq_server() is disabled,
and eventually if that function returns an error.
---
 exec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/exec.c b/exec.c
index 4f5df07b6a..0d30e48571 100644
--- a/exec.c
+++ b/exec.c
@@ -3318,7 +3318,8 @@ MemTxResult flatview_read_continue(FlatView *fv, hwaddr addr,
         } else {
             /* RAM case */
             ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
-            memcpy(buf, ptr, l);
+	    if (ptr)
+                memcpy(buf, ptr, l);
         }
 
         if (release_lock) {

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]

Am Thu,  9 Aug 2018 16:14:03 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

> The codepaths behind qemu_ram_ptr_length can return NULL.

While that might be a bug by itself, the question is why in that case no memset(buf, 0xff, l) is done?

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 485 bytes --]

On 09/08/2018 16:24, Olaf Hering wrote:
> Am Thu,  9 Aug 2018 16:14:03 +0200 schrieb Olaf Hering
> <olaf@aepfle.de>:
> 
>> The codepaths behind qemu_ram_ptr_length can return NULL.
>
> While that might be a bug by itself, the question is why in that case
> no memset(buf, 0xff, l) is done?

If no RAM is allocated (i.e. xen_map_cache will return NULL), however,
the memory should not be registered as RAM with the memory API.  So I
think the bug is in Xen code.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

Am Thu, 9 Aug 2018 16:37:05 +0200
schrieb Paolo Bonzini <pbonzini@redhat.com>:

> If no RAM is allocated (i.e. xen_map_cache will return NULL), however,
> the memory should not be registered as RAM with the memory API.  So I
> think the bug is in Xen code.

Someone familiar with that code has to figure that out. A ballooned page will trigger that bug.

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 2932 bytes --]

Am Thu, 9 Aug 2018 16:38:16 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

> Someone familiar with that code has to figure that out. A ballooned page will trigger that bug.

Indeed, xen-4.4 + qemu-3.0 crashes with ballooned pages. That can easily happen if the domU does readdir via NFS.

Olaf

Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
0x00007f439593f2ee in __memcpy_sse2_unaligned () from /lib64/libc.so.6
#0  0x00007f439593f2ee in __memcpy_sse2_unaligned () at /lib64/libc.so.6
#1  0x000055c7f7c8ee14 in memcpy (__len=1, __src=<optimized out>, __dest=0x7fff6819bc68) at /usr/include/bits/string3.h:53
#2  0x000055c7f7c8ee14 in flatview_read_continue (fv=0x55c7f99350f0, addr=3833593856, attrs=..., buf=0x7fff6819bc68 "", len=1, addr1=3833593856, l=1, mr=0x55c7f88309a0 <ram_memory>)
    at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/exec.c:3321
#3  0x000055c7f7c8efef in flatview_read (fv=0x55c7f99350f0, addr=3833593856, attrs=..., buf=0x7fff6819bc68 "", len=1) at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/exec.c:3354
#4  0x000055c7f7c8f11f in address_space_read_full (as=<optimized out>, addr=<optimized out>, attrs=..., buf=<optimized out>, len=<optimized out>)
    at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/exec.c:3367
#5  0x000055c7f7c8f337 in cpu_physical_memory_rw (addr=<optimized out>, buf=<optimized out>, len=<optimized out>, is_write=<optimized out>)
    at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/exec.c:3404
#6  0x000055c7f7d980a6 in read_phys_req_item (val=0x7fff6819bc68, i=0, req=0x7fff6819bc60, addr=<optimized out>)
    at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/hw/i386/xen/xen-hvm.c:841
#7  0x000055c7f7d980a6 in cpu_ioreq_move (req=0x7fff6819bc60) at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/hw/i386/xen/xen-hvm.c:904
#8  0x000055c7f7d980a6 in handle_ioreq (state=<optimized out>, req=0x7fff6819bc60) at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/hw/i386/xen/xen-hvm.c:1046
#9  0x000055c7f7d99b85 in cpu_handle_ioreq (opaque=0x55c7f90fe360) at /usr/src/debug/qemu-3.0-20180807T172617.6ad9080538/hw/i386/xen/xen-hvm.c:1153
#10 0x000055c7f811e288 in aio_dispatch_handlers (ctx=0x55c7f9052130) at util/aio-posix.c:406
#11 0x000055c7f811ec48 in aio_dispatch (ctx=0x55c7f9052130) at util/aio-posix.c:437
#12 0x000055c7f811a75e in aio_ctx_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at util/async.c:261
#13 0x00007f43965d6134 in g_main_context_dispatch () at /usr/lib64/libglib-2.0.so.0
#14 0x000055c7f811dca7 in glib_pollfds_poll () at util/main-loop.c:215
#15 0x000055c7f811dca7 in os_host_main_loop_wait (timeout=<optimized out>) at util/main-loop.c:238
#16 0x000055c7f811dca7 in main_loop_wait (nonblocking=<optimized out>) at util/main-loop.c:497
#17 0x000055c7f7e129c2 in main_loop () at vl.c:1866
#18 0x000055c7f7c7efdc in main ()

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

On 09/08/2018 16:38, Olaf Hering wrote:
> Am Thu, 9 Aug 2018 16:37:05 +0200
> schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
>> If no RAM is allocated (i.e. xen_map_cache will return NULL), however,
>> the memory should not be registered as RAM with the memory API.  So I
>> think the bug is in Xen code.
> 
> Someone familiar with that code has to figure that out. A ballooned page will trigger that bug.

I guess that's the answer.  I think the simplest fix is for the map
cache to set aside a zero page and return it whenever it is asked for a
ballooned page.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

Am Thu, 9 Aug 2018 16:52:22 +0200
schrieb Paolo Bonzini <pbonzini@redhat.com>:

> I think the simplest fix is for the map
> cache to set aside a zero page and return it whenever it is asked for a
> ballooned page.

Can qemu actually know if it ran into a ballooned page? I think no.

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 377 bytes --]

On 09/08/2018 16:55, Olaf Hering wrote:
> 
>> I think the simplest fix is for the map
>> cache to set aside a zero page and return it whenever it is asked for a
>> ballooned page.
> Can qemu actually know if it ran into a ballooned page? I think no.

Well, xen_map_cache knows that it has run into *something like* a
ballooned page when it returns NULL. :)

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Paolo Bonzini]

On 09/08/2018 17:03, Paolo Bonzini wrote:
> On 09/08/2018 16:55, Olaf Hering wrote:
>>
>>> I think the simplest fix is for the map
>>> cache to set aside a zero page and return it whenever it is asked for a
>>> ballooned page.
>> Can qemu actually know if it ran into a ballooned page? I think no.
> 
> Well, xen_map_cache knows that it has run into *something like* a
> ballooned page when it returns NULL. :)

... however, that works for reading to the page, not writing.  The
problem is that your patch is incomplete.  There are many more callers
of qemu_ram_ptr_length, and none of them check the result.

Paolo

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 277 bytes --]

Am Fri, 10 Aug 2018 12:25:09 +0200
schrieb Paolo Bonzini <pbonzini@redhat.com>:

> There are many more callers of qemu_ram_ptr_length, and none of them check the result.

So you mean that function must not return NULL?
Or should the caller check for the result?

Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

On 10/08/2018 12:32, Olaf Hering wrote:
> Am Fri, 10 Aug 2018 12:25:09 +0200
> schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
>> There are many more callers of qemu_ram_ptr_length, and none of them check the result.
> 
> So you mean that function must not return NULL?
> Or should the caller check for the result?

Either, but the former would of course be much simpler if possible.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

Am Fri, 10 Aug 2018 14:29:28 +0200
schrieb Paolo Bonzini <pbonzini@redhat.com>:

> On 10/08/2018 12:32, Olaf Hering wrote:
> > Am Fri, 10 Aug 2018 12:25:09 +0200
> > schrieb Paolo Bonzini <pbonzini@redhat.com>:
> > So you mean that function must not return NULL?
> > Or should the caller check for the result?  
> Either, but the former would of course be much simpler if possible.

Since other callers already deal with NULL, that one has to cope as well.
And since unavailable memory can not be zero, the memset 0xff is most likely correct as well.

So, for me the patch is good as it is. The write part will get a separate change in a few days.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

On 13/08/2018 22:07, Olaf Hering wrote:
> Since other callers already deal with NULL, that one has to cope as well.

They don't.

- address_space_map just returns the value that qemu_ram_ptr_length
returned, and none of its callers deal with NULL (there are dozens)

- likewise for address_space_cache_init

There is also memory_region_get_ram_ptr (which via qemu_map_ram_ptr is
also using xen_map_cache), and none of its callers deal with NULL either.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180809141403.11296-1-olaf@aepfle.de
Subject: [Qemu-devel] [PATCH v1] exec: handle NULL pointer in flatview_read_continue

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
41d94e5a05 exec: handle NULL pointer in flatview_read_continue

=== OUTPUT BEGIN ===
Checking PATCH 1/1: exec: handle NULL pointer in flatview_read_continue...
ERROR: code indent should never use tabs
#21: FILE: exec.c:3321:
+^I    if (ptr)$

total: 1 errors, 0 warnings, 9 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com