[Qemu-devel] [PATCH for-1.1] user-exec.c: Don't assert on segfaults for non-valid addresses

[Qemu-devel] [PATCH for-1.1] user-exec.c: Don't assert on segfaults for non-valid addresses

[Peter Maydell]

h2g() will assert if passed an address that's not a valid guest address,
so handle_cpu_signal() needs to check before passing "data address
which caused a segfault" to it, since for a misbehaving guest
that could be anything. If the address isn't a valid guest address
then we can simply skip the attempt to unprotect a guest page
which was made read-only to catch self-modifying code.

This assertion probably fires more readily now than it used to
do because of recent changes to default to reserving guest address
space.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I've tentatively marked this as for-1.1 as it's pretty safe, although
it doesn't buy you a great deal: misbehaving guest binaries will
die cleanly with a segfault rather than qemu asserting and then
locking up (assert() in qemu's linux-user code doesn't really behave
very nicely...)

 user-exec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/user-exec.c b/user-exec.c
index be6bc4f..d8c2ad9 100644
--- a/user-exec.c
+++ b/user-exec.c
@@ -97,7 +97,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
                 pc, address, is_write, *(unsigned long *)old_set);
 #endif
     /* XXX: locking issue */
-    if (is_write && page_unprotect(h2g(address), pc, puc)) {
+    if (is_write && h2g_valid(address)
+        && page_unprotect(h2g(address), pc, puc)) {
         return 1;
     }
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH for-1.1] user-exec.c: Don't assert on segfaults for non-valid addresses

[Alexander Graf]

On 03.05.2012, at 20:32, Peter Maydell wrote:

> h2g() will assert if passed an address that's not a valid guest address,
> so handle_cpu_signal() needs to check before passing "data address
> which caused a segfault" to it, since for a misbehaving guest
> that could be anything. If the address isn't a valid guest address
> then we can simply skip the attempt to unprotect a guest page
> which was made read-only to catch self-modifying code.
> 
> This assertion probably fires more readily now than it used to
> do because of recent changes to default to reserving guest address
> space.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Yup, just wrote the same thing a few hours ago.

Acked-by: Alexander Graf <agraf@suse.de>

> ---
> I've tentatively marked this as for-1.1 as it's pretty safe, although
> it doesn't buy you a great deal: misbehaving guest binaries will
> die cleanly with a segfault rather than qemu asserting and then
> locking up (assert() in qemu's linux-user code doesn't really behave
> very nicely...)

It's definitely 1.1 material.


Alex

Re: [Qemu-devel] [PATCH for-1.1] user-exec.c: Don't assert on segfaults for non-valid addresses

[Anthony Liguori]

On 05/03/2012 01:32 PM, Peter Maydell wrote:
> h2g() will assert if passed an address that's not a valid guest address,
> so handle_cpu_signal() needs to check before passing "data address
> which caused a segfault" to it, since for a misbehaving guest
> that could be anything. If the address isn't a valid guest address
> then we can simply skip the attempt to unprotect a guest page
> which was made read-only to catch self-modifying code.
>
> This assertion probably fires more readily now than it used to
> do because of recent changes to default to reserving guest address
> space.
>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
> I've tentatively marked this as for-1.1 as it's pretty safe, although
> it doesn't buy you a great deal: misbehaving guest binaries will
> die cleanly with a segfault rather than qemu asserting and then
> locking up (assert() in qemu's linux-user code doesn't really behave
> very nicely...)
>
>   user-exec.c |    3 ++-
>   1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/user-exec.c b/user-exec.c
> index be6bc4f..d8c2ad9 100644
> --- a/user-exec.c
> +++ b/user-exec.c
> @@ -97,7 +97,8 @@ static inline int handle_cpu_signal(uintptr_t pc, unsigned long address,
>                   pc, address, is_write, *(unsigned long *)old_set);
>   #endif
>       /* XXX: locking issue */
> -    if (is_write&&  page_unprotect(h2g(address), pc, puc)) {
> +    if (is_write&&  h2g_valid(address)
> +&&  page_unprotect(h2g(address), pc, puc)) {
>           return 1;
>       }
>