[Qemu-devel] [PATCH] Fix changing password using monitor over VNC.

[Qemu-devel] [PATCH] Fix changing password using monitor over VNC.

[Zachary Amsden]

A simple segfault turned out to be a relatively complex fix.

The monitor calls back to main_loop_wait() to wait for the completion
of the password change event; this results in a nested invocation of
the associated I/O handlers.  For stdio monitor, this is okay, but VNC
maintains an input buffer which is not flushed until after the
invocation of protocol actions.  This is non-reentrant; the result is
that the nested invocation consumes the same protocol event as the
parent (which was a '\n', setting a NULL password), and it gets worse
when both the child and the parent attempt to shift in the same input
event, resulting in a memmove of size -1ULL, and a segfault.

The fix is to consume the input buffer before invoking protocol actions
which may cause nested invocation of the handler; we must also set up
the child handler to receive new events, which was cleanest done with
vnc_read_when() from the protcol handler (doing it in the outer loop
causes bugs with other types of waits, such as auth).  We return fed=1
from the outer handler to prevent the logic in vnc_client_read from
reconsuming the pre-consumed buffer, and simply reset the expect
value to receive the next protocol command.

Signed-off-by: Zachary Amsden <zamsden@redhat.com>
---
 vnc.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/vnc.c b/vnc.c
index 2b3a6eb..3e88855 100644
--- a/vnc.c
+++ b/vnc.c
@@ -798,6 +798,15 @@ static void vnc_read_when(VncState *vs, VncReadEvent *func, size_t expecting)
     vs->read_handler_expect = expecting;
 }
 
+static void vnc_client_consume(VncState *vs, size_t len)
+{
+    if (vs->input.offset >= len) {
+        memmove(vs->input.buffer, vs->input.buffer + len,
+                (vs->input.offset - len));
+        vs->input.offset -= len;
+    }
+}
+
 static void vnc_client_read(void *opaque)
 {
     VncState *vs = opaque;
@@ -833,8 +842,7 @@ static void vnc_client_read(void *opaque)
 	    return;
 
 	if (!ret) {
-	    memmove(vs->input.buffer, vs->input.buffer + len, (vs->input.offset - len));
-	    vs->input.offset -= len;
+            vnc_client_consume(vs, len);
 	} else {
 	    vs->read_handler_expect = ret;
 	}
@@ -1357,6 +1365,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
 {
     int i;
     uint16_t limit;
+    int fed = 0;
 
     switch (data[0]) {
     case 0:
@@ -1399,6 +1408,9 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
 	if (len == 1)
 	    return 8;
 
+        vnc_client_consume(vs, len);
+        vnc_read_when(vs, protocol_client_msg, 1);
+        fed = 1;
 	key_event(vs, read_u8(data, 1), read_u32(data, 4));
 	break;
     case 5:
@@ -1428,6 +1440,9 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
             if (len == 2)
                 return 12;
 
+            vnc_client_consume(vs, len);
+            vnc_read_when(vs, protocol_client_msg, 1);
+            fed = 1;
             ext_key_event(vs, read_u16(data, 2),
                           read_u32(data, 4), read_u32(data, 8));
             break;
@@ -1486,7 +1501,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
     }
 
     vnc_read_when(vs, protocol_client_msg, 1);
-    return 0;
+    return fed;
 }
 
 static int protocol_client_init(VncState *vs, uint8_t *data, size_t len)
@@ -2285,6 +2300,8 @@ int vnc_display_password(DisplayState *ds, const char *password)
 	if (!(vs->password = qemu_strdup(password)))
 	    return -1;
     }
+    if (vs->auth == VNC_AUTH_NONE)
+        vs->auth = VNC_AUTH_VNC;
 
     return 0;
 }
-- 
1.6.2.2.471.g6da14

[Qemu-devel] Re: [PATCH] Fix changing password using monitor over VNC.

[Jan Kiszka]

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

Zachary Amsden wrote:
> A simple segfault turned out to be a relatively complex fix.
> 
> The monitor calls back to main_loop_wait() to wait for the completion
> of the password change event; this results in a nested invocation of

This is no longer true with trunk as this nasty blocking password
reading has been converted into an async operation. Is your patch
required nevertheless? Or is this band-aid for stable?

> the associated I/O handlers.  For stdio monitor, this is okay, but VNC
> maintains an input buffer which is not flushed until after the
> invocation of protocol actions.  This is non-reentrant; the result is
> that the nested invocation consumes the same protocol event as the
> parent (which was a '\n', setting a NULL password), and it gets worse
> when both the child and the parent attempt to shift in the same input
> event, resulting in a memmove of size -1ULL, and a segfault.
> 
> The fix is to consume the input buffer before invoking protocol actions
> which may cause nested invocation of the handler; we must also set up
> the child handler to receive new events, which was cleanest done with
> vnc_read_when() from the protcol handler (doing it in the outer loop
> causes bugs with other types of waits, such as auth).  We return fed=1
> from the outer handler to prevent the logic in vnc_client_read from
> reconsuming the pre-consumed buffer, and simply reset the expect
> value to receive the next protocol command.
> 
> Signed-off-by: Zachary Amsden <zamsden@redhat.com>
> ---

Jan


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

[Qemu-devel] Re: [PATCH] Fix changing password using monitor over VNC.

[Zachary Amsden]

Jan Kiszka wrote:
> Zachary Amsden wrote:
>> A simple segfault turned out to be a relatively complex fix.
>>
>> The monitor calls back to main_loop_wait() to wait for the completion
>> of the password change event; this results in a nested invocation of
> 
> This is no longer true with trunk as this nasty blocking password
> reading has been converted into an async operation. Is your patch
> required nevertheless? Or is this band-aid for stable?

I confirmed the bug, verified the fix on a git tree sync'd today from
git://git.savannah.nongnu.org/qemu.git

let me know if this is not the trunk.

Sorry, as I am new to qemu development.

Cheers,

Zach

[Qemu-devel] Re: [PATCH] Fix changing password using monitor over VNC.

[Jan Kiszka]

Zachary Amsden wrote:
> Jan Kiszka wrote:
>> Zachary Amsden wrote:
>>> A simple segfault turned out to be a relatively complex fix.
>>>
>>> The monitor calls back to main_loop_wait() to wait for the completion
>>> of the password change event; this results in a nested invocation of
>> This is no longer true with trunk as this nasty blocking password
>> reading has been converted into an async operation. Is your patch
>> required nevertheless? Or is this band-aid for stable?
> 
> I confirmed the bug, verified the fix on a git tree sync'd today from
> git://git.savannah.nongnu.org/qemu.git
> 
> let me know if this is not the trunk.

Nope, it's outdated. Last check in is from end of January this year.

There are several up-to-date git mirrors, one e.g. at kernel.org:
git://git.kernel.org/pub/scm/virt/qemu/qemu.git. However, I prefer to do
my own mirroring with git svn.

> 
> Sorry, as I am new to qemu development.

No problem. The issue you found is probably still relevant for stable
0.10.x, so please consider this branch as well.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] Fix changing password using monitor over VNC.

[Zachary Amsden]

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

Jan Kiszka wrote:
> Zachary Amsden wrote:
>> Jan Kiszka wrote:
>>> Zachary Amsden wrote:
>>>> A simple segfault turned out to be a relatively complex fix.
>>>>
>>>> The monitor calls back to main_loop_wait() to wait for the completion
>>>> of the password change event; this results in a nested invocation of
>>> This is no longer true with trunk as this nasty blocking password
>>> reading has been converted into an async operation. Is your patch
>>> required nevertheless? Or is this band-aid for stable?
>> I confirmed the bug, verified the fix on a git tree sync'd today from
>> git://git.savannah.nongnu.org/qemu.git
>>
>> let me know if this is not the trunk.
> 
> Nope, it's outdated. Last check in is from end of January this year.
> 
> There are several up-to-date git mirrors, one e.g. at kernel.org:
> git://git.kernel.org/pub/scm/virt/qemu/qemu.git. However, I prefer to do
> my own mirroring with git svn.
> 
>> Sorry, as I am new to qemu development.
> 
> No problem. The issue you found is probably still relevant for stable
> 0.10.x, so please consider this branch as well.

The patch applies as is to stable branch.

And this new patch fixes two bugs on trunk:

1) Changing VNC password should set password auth if it wasn't yet enabled
2) Trying to change VNC password on a SDL-only display gives a segfault.



[-- Attachment #2: vnc-password-trunk.patch --]
[-- Type: text/plain, Size: 636 bytes --]

diff --git a/vnc.c b/vnc.c
index ab1f044..c49ce61 100644
--- a/vnc.c
+++ b/vnc.c
@@ -2082,6 +2082,9 @@ int vnc_display_password(DisplayState *ds, const char *password)
 {
     VncDisplay *vs = ds ? (VncDisplay *)ds->opaque : vnc_display;
 
+    if (!vs)
+        return -1;
+
     if (vs->password) {
         qemu_free(vs->password);
         vs->password = NULL;
@@ -2090,6 +2093,9 @@ int vnc_display_password(DisplayState *ds, const char *password)
         if (!(vs->password = qemu_strdup(password)))
             return -1;
     }
+    if (vs->auth == VNC_AUTH_NONE) {
+        vs->auth = VNC_AUTH_VNC;
+    }
 
     return 0;
 }

[Qemu-devel] Re: [PATCH] Fix changing password using monitor over VNC.

[Jan Kiszka]

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

Zachary Amsden wrote:
> Jan Kiszka wrote:
>> Zachary Amsden wrote:
>>> Jan Kiszka wrote:
>>>> Zachary Amsden wrote:
>>>>> A simple segfault turned out to be a relatively complex fix.
>>>>>
>>>>> The monitor calls back to main_loop_wait() to wait for the completion
>>>>> of the password change event; this results in a nested invocation of
>>>> This is no longer true with trunk as this nasty blocking password
>>>> reading has been converted into an async operation. Is your patch
>>>> required nevertheless? Or is this band-aid for stable?
>>> I confirmed the bug, verified the fix on a git tree sync'd today from
>>> git://git.savannah.nongnu.org/qemu.git
>>>
>>> let me know if this is not the trunk.
>> Nope, it's outdated. Last check in is from end of January this year.
>>
>> There are several up-to-date git mirrors, one e.g. at kernel.org:
>> git://git.kernel.org/pub/scm/virt/qemu/qemu.git. However, I prefer to do
>> my own mirroring with git svn.
>>
>>> Sorry, as I am new to qemu development.
>> No problem. The issue you found is probably still relevant for stable
>> 0.10.x, so please consider this branch as well.
> 
> The patch applies as is to stable branch.
> 
> And this new patch fixes two bugs on trunk:
> 
> 1) Changing VNC password should set password auth if it wasn't yet enabled
> 2) Trying to change VNC password on a SDL-only display gives a segfault.
> 

I thought I fixed something like 2) along the monitor rework, but
obviously not every case (VNC is not in daily use here).

However, your patch requires standard format:

[PATCH] <subject> (if it starts with 'Re:', it risks to be overseen)

<description>

Signed-off-by: ...

<diff>

Thanks,
Jan


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