* [Qemu-devel] [PATCH v2] ps2: check PS2Queue indices in post_load routine
@ 2017-11-16 10:16 P J P
2017-11-16 10:23 ` Paolo Bonzini
0 siblings, 1 reply; 2+ messages in thread
From: P J P @ 2017-11-16 10:16 UTC (permalink / raw)
To: Qemu Developers
Cc: Gerd Hoffmann, Daniel P . Berrange, Paolo Bonzini,
Cyrille Chatras, Dr . David Alan Gilbert, Prasad J Pandit
From: Prasad J Pandit <pjp@fedoraproject.org>
During Qemu guest migration, a destination process invokes ps2
post_load function. In that, if 'rptr' and 'count' values were
invalid, it could lead to OOB access issue. Add check to avoid
it.
Reported-by: Cyrille Chatras <cyrille.chatras@orange.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/input/ps2.c | 39 ++++++++++++++++++---------------------
1 file changed, 18 insertions(+), 21 deletions(-)
Update: return -1(bad indices) error, instead of correcting values
-> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg02964.html
diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index f388a23c8e..35392d55fc 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -1222,32 +1222,37 @@ static void ps2_common_reset(PS2State *s)
s->update_irq(s->update_arg, 0);
}
-static void ps2_common_post_load(PS2State *s)
+static int ps2_common_post_load(PS2State *s)
{
PS2Queue *q = &s->queue;
int size;
int i;
int tmp_data[PS2_QUEUE_SIZE];
+ if (q->count < 0 || q->rptr < 0 || q->rptr >= sizeof(q->data)) {
+ return -1;
+ }
+
/* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
size = q->count > PS2_QUEUE_SIZE ? 0 : q->count;
/* move the queue elements to the start of data array */
- if (size > 0) {
- for (i = 0; i < size; i++) {
- /* move the queue elements to the temporary buffer */
- tmp_data[i] = q->data[q->rptr];
- if (++q->rptr == 256) {
- q->rptr = 0;
- }
+ for (i = 0; i < size; i++) {
+ /* move the queue elements to the temporary buffer */
+ tmp_data[i] = q->data[q->rptr];
+ if (++q->rptr == 256) {
+ q->rptr = 0;
}
- memcpy(q->data, tmp_data, size);
}
+ memcpy(q->data, tmp_data, size);
+
/* reset rptr/wptr/count */
q->rptr = 0;
q->wptr = size;
q->count = size;
s->update_irq(s->update_arg, q->count != 0);
+
+ return 0;
}
static void ps2_kbd_reset(void *opaque)
@@ -1346,9 +1351,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
if (version_id == 2)
s->scancode_set=2;
- ps2_common_post_load(ps2);
-
- return 0;
+ return ps2_common_post_load(ps2);
}
static int ps2_kbd_pre_save(void *opaque)
@@ -1356,9 +1359,7 @@ static int ps2_kbd_pre_save(void *opaque)
PS2KbdState *s = (PS2KbdState *)opaque;
PS2State *ps2 = &s->common;
- ps2_common_post_load(ps2);
-
- return 0;
+ return ps2_common_post_load(ps2);
}
static const VMStateDescription vmstate_ps2_keyboard = {
@@ -1386,9 +1387,7 @@ static int ps2_mouse_post_load(void *opaque, int version_id)
PS2MouseState *s = (PS2MouseState *)opaque;
PS2State *ps2 = &s->common;
- ps2_common_post_load(ps2);
-
- return 0;
+ return ps2_common_post_load(ps2);
}
static int ps2_mouse_pre_save(void *opaque)
@@ -1396,9 +1395,7 @@ static int ps2_mouse_pre_save(void *opaque)
PS2MouseState *s = (PS2MouseState *)opaque;
PS2State *ps2 = &s->common;
- ps2_common_post_load(ps2);
-
- return 0;
+ return ps2_common_post_load(ps2);
}
static const VMStateDescription vmstate_ps2_mouse = {
--
2.13.6
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH v2] ps2: check PS2Queue indices in post_load routine
2017-11-16 10:16 [Qemu-devel] [PATCH v2] ps2: check PS2Queue indices in post_load routine P J P
@ 2017-11-16 10:23 ` Paolo Bonzini
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Bonzini @ 2017-11-16 10:23 UTC (permalink / raw)
To: P J P, Qemu Developers
Cc: Gerd Hoffmann, Daniel P . Berrange, Cyrille Chatras,
Dr . David Alan Gilbert, Prasad J Pandit
On 16/11/2017 11:16, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> During Qemu guest migration, a destination process invokes ps2
> post_load function. In that, if 'rptr' and 'count' values were
> invalid, it could lead to OOB access issue. Add check to avoid
> it.
>
> Reported-by: Cyrille Chatras <cyrille.chatras@orange.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/input/ps2.c | 39 ++++++++++++++++++---------------------
> 1 file changed, 18 insertions(+), 21 deletions(-)
>
> Update: return -1(bad indices) error, instead of correcting values
> -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg02964.html
>
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> index f388a23c8e..35392d55fc 100644
> --- a/hw/input/ps2.c
> +++ b/hw/input/ps2.c
> @@ -1222,32 +1222,37 @@ static void ps2_common_reset(PS2State *s)
> s->update_irq(s->update_arg, 0);
> }
>
> -static void ps2_common_post_load(PS2State *s)
> +static int ps2_common_post_load(PS2State *s)
> {
> PS2Queue *q = &s->queue;
> int size;
> int i;
> int tmp_data[PS2_QUEUE_SIZE];
>
> + if (q->count < 0 || q->rptr < 0 || q->rptr >= sizeof(q->data)) {
> + return -1;
> + }
> +
> /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
> size = q->count > PS2_QUEUE_SIZE ? 0 : q->count;
>
> /* move the queue elements to the start of data array */
> - if (size > 0) {
> - for (i = 0; i < size; i++) {
> - /* move the queue elements to the temporary buffer */
> - tmp_data[i] = q->data[q->rptr];
> - if (++q->rptr == 256) {
> - q->rptr = 0;
> - }
> + for (i = 0; i < size; i++) {
> + /* move the queue elements to the temporary buffer */
> + tmp_data[i] = q->data[q->rptr];
> + if (++q->rptr == 256) {
> + q->rptr = 0;
> }
> - memcpy(q->data, tmp_data, size);
> }
> + memcpy(q->data, tmp_data, size);
> +
> /* reset rptr/wptr/count */
> q->rptr = 0;
> q->wptr = size;
> q->count = size;
> s->update_irq(s->update_arg, q->count != 0);
> +
> + return 0;
> }
>
> static void ps2_kbd_reset(void *opaque)
> @@ -1346,9 +1351,7 @@ static int ps2_kbd_post_load(void* opaque, int version_id)
> if (version_id == 2)
> s->scancode_set=2;
>
> - ps2_common_post_load(ps2);
> -
> - return 0;
> + return ps2_common_post_load(ps2);
> }
>
> static int ps2_kbd_pre_save(void *opaque)
> @@ -1356,9 +1359,7 @@ static int ps2_kbd_pre_save(void *opaque)
> PS2KbdState *s = (PS2KbdState *)opaque;
> PS2State *ps2 = &s->common;
>
> - ps2_common_post_load(ps2);
> -
> - return 0;
> + return ps2_common_post_load(ps2);
> }
>
> static const VMStateDescription vmstate_ps2_keyboard = {
> @@ -1386,9 +1387,7 @@ static int ps2_mouse_post_load(void *opaque, int version_id)
> PS2MouseState *s = (PS2MouseState *)opaque;
> PS2State *ps2 = &s->common;
>
> - ps2_common_post_load(ps2);
> -
> - return 0;
> + return ps2_common_post_load(ps2);
> }
>
> static int ps2_mouse_pre_save(void *opaque)
> @@ -1396,9 +1395,7 @@ static int ps2_mouse_pre_save(void *opaque)
> PS2MouseState *s = (PS2MouseState *)opaque;
> PS2State *ps2 = &s->common;
>
> - ps2_common_post_load(ps2);
> -
> - return 0;
> + return ps2_common_post_load(ps2);
> }
>
> static const VMStateDescription vmstate_ps2_mouse = {
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-11-16 10:23 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-16 10:16 [Qemu-devel] [PATCH v2] ps2: check PS2Queue indices in post_load routine P J P
2017-11-16 10:23 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).