* [Qemu-devel] [PATCH 0/4] Misc VNC fixes / sanity checks
@ 2018-02-05 11:49 Daniel P. Berrangé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check Daniel P. Berrangé
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2018-02-05 11:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Laszlo Ersek, Daniel P. Berrangé
A few VNC changes suggested by Laszlo when reviewing my recent VNC patches
Daniel P. Berrangé (4):
ui: avoid risk of 32-bit int overflow in VNC buffer check
ui: avoid 'local_err' variable shadowing in VNC SASL auth
ui: check VNC audio frequency limit at time of reading from client
ui: extend VNC trottling tracing to SASL codepaths
ui/vnc-auth-sasl.c | 13 +++++++++++--
ui/vnc.c | 26 +++++++++++++++-----------
2 files changed, 26 insertions(+), 13 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check
2018-02-05 11:49 [Qemu-devel] [PATCH 0/4] Misc VNC fixes / sanity checks Daniel P. Berrangé
@ 2018-02-05 11:49 ` Daniel P. Berrangé
2018-02-05 12:58 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth Daniel P. Berrangé
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2018-02-05 11:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Laszlo Ersek, Daniel P. Berrangé
For very large framebuffers, it is theoretically possible for the result
of 'vs->throttle_output_offset * VNC_THROTTLE_OUTPUT_LIMIT_SCALE' to
exceed the size of a 32-bit int. For this to happen in practice, the
video RAM would have to be set to a large enough value, which is not
likely today. None the less we can be paranoid against future growth by
using division instead of multiplication when checking the limits.
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
ui/vnc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index 93731accb6..e14e524764 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1572,8 +1572,8 @@ void vnc_write(VncState *vs, const void *data, size_t len)
* handshake, or from the job thread's VncState clone
*/
if (vs->throttle_output_offset != 0 &&
- vs->output.offset > (vs->throttle_output_offset *
- VNC_THROTTLE_OUTPUT_LIMIT_SCALE)) {
+ (vs->output.offset / VNC_THROTTLE_OUTPUT_LIMIT_SCALE) >
+ vs->throttle_output_offset) {
trace_vnc_client_output_limit(vs, vs->ioc, vs->output.offset,
vs->throttle_output_offset);
vnc_disconnect_start(vs);
--
2.14.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth
2018-02-05 11:49 [Qemu-devel] [PATCH 0/4] Misc VNC fixes / sanity checks Daniel P. Berrangé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check Daniel P. Berrangé
@ 2018-02-05 11:49 ` Daniel P. Berrangé
2018-02-05 12:58 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client Daniel P. Berrangé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 4/4] ui: extend VNC trottling tracing to SASL codepaths Daniel P. Berrangé
3 siblings, 2 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2018-02-05 11:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Laszlo Ersek, Daniel P. Berrangé
The start_auth_sasl() method declares a 'Error *local_err' variable in
an inner if () {...} scope, which shadows a variable of the same name
declared at the start of the method. This is confusing for reviewers and
may trigger compiler warnings.
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
ui/vnc-auth-sasl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index fbccca8c8a..8ebd0d3d00 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -556,7 +556,6 @@ void start_auth_sasl(VncState *vs)
/* Inform SASL that we've got an external SSF layer from TLS/x509 */
if (vs->auth == VNC_AUTH_VENCRYPT &&
vs->subauth == VNC_AUTH_VENCRYPT_X509SASL) {
- Error *local_err = NULL;
int keysize;
sasl_ssf_t ssf;
@@ -565,7 +564,6 @@ void start_auth_sasl(VncState *vs)
if (keysize < 0) {
trace_vnc_auth_fail(vs, vs->auth, "cannot TLS get cipher size",
error_get_pretty(local_err));
- error_free(local_err);
sasl_dispose(&vs->sasl.conn);
vs->sasl.conn = NULL;
goto authabort;
--
2.14.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client
2018-02-05 11:49 [Qemu-devel] [PATCH 0/4] Misc VNC fixes / sanity checks Daniel P. Berrangé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check Daniel P. Berrangé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth Daniel P. Berrangé
@ 2018-02-05 11:49 ` Daniel P. Berrangé
2018-02-05 13:03 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 4/4] ui: extend VNC trottling tracing to SASL codepaths Daniel P. Berrangé
3 siblings, 2 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2018-02-05 11:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Laszlo Ersek, Daniel P. Berrangé
The 'vs->as.freq' value is a signed integer, which is read from an
unsigned 32-bit int field on the wire. There is thus a risk of overflow
on 32-bit platforms. Move the frequency limit checking to be done at
time of read before casting to a signed integer.
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
ui/vnc.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/ui/vnc.c b/ui/vnc.c
index e14e524764..79ac9eccde 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -982,14 +982,7 @@ static void vnc_update_throttle_offset(VncState *vs)
vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
if (vs->audio_cap) {
- int freq = vs->as.freq;
- /* We don't limit freq when reading settings from client, so
- * it could be upto MAX_INT in size. 48khz is a sensible
- * upper bound for trustworthy clients */
int bps;
- if (freq > 48000) {
- freq = 48000;
- }
switch (vs->as.fmt) {
default:
case AUD_FMT_U8:
@@ -1005,7 +998,7 @@ static void vnc_update_throttle_offset(VncState *vs)
bps = 4;
break;
}
- offset += freq * bps * vs->as.nchannels;
+ offset += vs->as.freq * bps * vs->as.nchannels;
}
/* Put a floor of 1MB on offset, so that if we have a large pending
@@ -2279,6 +2272,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
{
int i;
uint16_t limit;
+ uint32_t freq;
VncDisplay *vd = vs->vd;
if (data[0] > 3) {
@@ -2398,7 +2392,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
vnc_client_error(vs);
break;
}
- vs->as.freq = read_u32(data, 6);
+ freq = read_u32(data, 6);
+ /* No official limit for protocol, but 48khz is a sensible
+ * upper bound for trustworthy clients, and this limit
+ * protects calculations involving 'vs->as.freq' later.
+ */
+ if (freq > 48000) {
+ VNC_DEBUG("Invalid audio frequency %u > 48000", freq);
+ vnc_client_error(vs);
+ break;
+ }
+ vs->as.freq = freq;
break;
default:
VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
--
2.14.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 4/4] ui: extend VNC trottling tracing to SASL codepaths
2018-02-05 11:49 [Qemu-devel] [PATCH 0/4] Misc VNC fixes / sanity checks Daniel P. Berrangé
` (2 preceding siblings ...)
2018-02-05 11:49 ` [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client Daniel P. Berrangé
@ 2018-02-05 11:49 ` Daniel P. Berrangé
2018-02-05 13:08 ` Laszlo Ersek
3 siblings, 1 reply; 12+ messages in thread
From: Daniel P. Berrangé @ 2018-02-05 11:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Gerd Hoffmann, Laszlo Ersek, Daniel P. Berrangé
In previous commit:
commit 6aa22a29187e1908f5db738d27c64a9efc8d0bfa
Author: Daniel P. Berrange <berrange@redhat.com>
Date: Mon Dec 18 19:12:27 2017 +0000
ui: add trace events related to VNC client throttling
trace points related to unthrottling client I/O were missed from the
SASL codepaths.
Reported-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
ui/vnc-auth-sasl.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 8ebd0d3d00..3751a777a4 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -79,12 +79,23 @@ size_t vnc_client_write_sasl(VncState *vs)
vs->sasl.encodedOffset += ret;
if (vs->sasl.encodedOffset == vs->sasl.encodedLength) {
+ bool throttled = vs->force_update_offset != 0;
+ size_t offset;
if (vs->sasl.encodedRawLength >= vs->force_update_offset) {
vs->force_update_offset = 0;
} else {
vs->force_update_offset -= vs->sasl.encodedRawLength;
}
+ if (throttled && vs->force_update_offset == 0) {
+ trace_vnc_client_unthrottle_forced(vs, vs->ioc);
+ }
+ offset = vs->output.offset;
buffer_advance(&vs->output, vs->sasl.encodedRawLength);
+ if (offset >= vs->throttle_output_offset &&
+ vs->output.offset < vs->throttle_output_offset) {
+ trace_vnc_client_unthrottle_incremental(vs, vs->ioc,
+ vs->output.offset);
+ }
vs->sasl.encoded = NULL;
vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;
}
--
2.14.3
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check
2018-02-05 11:49 ` [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check Daniel P. Berrangé
@ 2018-02-05 12:58 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2018-02-05 12:58 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Gerd Hoffmann
On 02/05/18 12:49, Daniel P. Berrangé wrote:
> For very large framebuffers, it is theoretically possible for the result
> of 'vs->throttle_output_offset * VNC_THROTTLE_OUTPUT_LIMIT_SCALE' to
> exceed the size of a 32-bit int. For this to happen in practice, the
> video RAM would have to be set to a large enough value, which is not
> likely today. None the less we can be paranoid against future growth by
> using division instead of multiplication when checking the limits.
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> ui/vnc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 93731accb6..e14e524764 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1572,8 +1572,8 @@ void vnc_write(VncState *vs, const void *data, size_t len)
> * handshake, or from the job thread's VncState clone
> */
> if (vs->throttle_output_offset != 0 &&
> - vs->output.offset > (vs->throttle_output_offset *
> - VNC_THROTTLE_OUTPUT_LIMIT_SCALE)) {
> + (vs->output.offset / VNC_THROTTLE_OUTPUT_LIMIT_SCALE) >
> + vs->throttle_output_offset) {
> trace_vnc_client_output_limit(vs, vs->ioc, vs->output.offset,
> vs->throttle_output_offset);
> vnc_disconnect_start(vs);
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth
2018-02-05 11:49 ` [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth Daniel P. Berrangé
@ 2018-02-05 12:58 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2018-02-05 12:58 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Gerd Hoffmann
On 02/05/18 12:49, Daniel P. Berrangé wrote:
> The start_auth_sasl() method declares a 'Error *local_err' variable in
> an inner if () {...} scope, which shadows a variable of the same name
> declared at the start of the method. This is confusing for reviewers and
> may trigger compiler warnings.
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> ui/vnc-auth-sasl.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
> index fbccca8c8a..8ebd0d3d00 100644
> --- a/ui/vnc-auth-sasl.c
> +++ b/ui/vnc-auth-sasl.c
> @@ -556,7 +556,6 @@ void start_auth_sasl(VncState *vs)
> /* Inform SASL that we've got an external SSF layer from TLS/x509 */
> if (vs->auth == VNC_AUTH_VENCRYPT &&
> vs->subauth == VNC_AUTH_VENCRYPT_X509SASL) {
> - Error *local_err = NULL;
> int keysize;
> sasl_ssf_t ssf;
>
> @@ -565,7 +564,6 @@ void start_auth_sasl(VncState *vs)
> if (keysize < 0) {
> trace_vnc_auth_fail(vs, vs->auth, "cannot TLS get cipher size",
> error_get_pretty(local_err));
> - error_free(local_err);
> sasl_dispose(&vs->sasl.conn);
> vs->sasl.conn = NULL;
> goto authabort;
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client
2018-02-05 11:49 ` [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client Daniel P. Berrangé
@ 2018-02-05 13:03 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2018-02-05 13:03 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Gerd Hoffmann
On 02/05/18 12:49, Daniel P. Berrangé wrote:
> The 'vs->as.freq' value is a signed integer, which is read from an
> unsigned 32-bit int field on the wire. There is thus a risk of overflow
> on 32-bit platforms. Move the frequency limit checking to be done at
> time of read before casting to a signed integer.
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> ui/vnc.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e14e524764..79ac9eccde 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -982,14 +982,7 @@ static void vnc_update_throttle_offset(VncState *vs)
> vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
>
> if (vs->audio_cap) {
> - int freq = vs->as.freq;
> - /* We don't limit freq when reading settings from client, so
> - * it could be upto MAX_INT in size. 48khz is a sensible
> - * upper bound for trustworthy clients */
> int bps;
> - if (freq > 48000) {
> - freq = 48000;
> - }
> switch (vs->as.fmt) {
> default:
> case AUD_FMT_U8:
> @@ -1005,7 +998,7 @@ static void vnc_update_throttle_offset(VncState *vs)
> bps = 4;
> break;
> }
> - offset += freq * bps * vs->as.nchannels;
> + offset += vs->as.freq * bps * vs->as.nchannels;
> }
>
> /* Put a floor of 1MB on offset, so that if we have a large pending
> @@ -2279,6 +2272,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
> {
> int i;
> uint16_t limit;
> + uint32_t freq;
> VncDisplay *vd = vs->vd;
>
> if (data[0] > 3) {
> @@ -2398,7 +2392,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
> vnc_client_error(vs);
> break;
> }
> - vs->as.freq = read_u32(data, 6);
> + freq = read_u32(data, 6);
> + /* No official limit for protocol, but 48khz is a sensible
> + * upper bound for trustworthy clients, and this limit
> + * protects calculations involving 'vs->as.freq' later.
> + */
> + if (freq > 48000) {
> + VNC_DEBUG("Invalid audio frequency %u > 48000", freq);
> + vnc_client_error(vs);
> + break;
> + }
> + vs->as.freq = freq;
> break;
> default:
> VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] ui: extend VNC trottling tracing to SASL codepaths
2018-02-05 11:49 ` [Qemu-devel] [PATCH 4/4] ui: extend VNC trottling tracing to SASL codepaths Daniel P. Berrangé
@ 2018-02-05 13:08 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2018-02-05 13:08 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Gerd Hoffmann
On 02/05/18 12:49, Daniel P. Berrangé wrote:
> In previous commit:
>
> commit 6aa22a29187e1908f5db738d27c64a9efc8d0bfa
> Author: Daniel P. Berrange <berrange@redhat.com>
> Date: Mon Dec 18 19:12:27 2017 +0000
>
> ui: add trace events related to VNC client throttling
>
> trace points related to unthrottling client I/O were missed from the
> SASL codepaths.
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> ui/vnc-auth-sasl.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
> index 8ebd0d3d00..3751a777a4 100644
> --- a/ui/vnc-auth-sasl.c
> +++ b/ui/vnc-auth-sasl.c
> @@ -79,12 +79,23 @@ size_t vnc_client_write_sasl(VncState *vs)
>
> vs->sasl.encodedOffset += ret;
> if (vs->sasl.encodedOffset == vs->sasl.encodedLength) {
> + bool throttled = vs->force_update_offset != 0;
> + size_t offset;
> if (vs->sasl.encodedRawLength >= vs->force_update_offset) {
> vs->force_update_offset = 0;
> } else {
> vs->force_update_offset -= vs->sasl.encodedRawLength;
> }
> + if (throttled && vs->force_update_offset == 0) {
> + trace_vnc_client_unthrottle_forced(vs, vs->ioc);
> + }
> + offset = vs->output.offset;
> buffer_advance(&vs->output, vs->sasl.encodedRawLength);
> + if (offset >= vs->throttle_output_offset &&
> + vs->output.offset < vs->throttle_output_offset) {
> + trace_vnc_client_unthrottle_incremental(vs, vs->ioc,
> + vs->output.offset);
> + }
> vs->sasl.encoded = NULL;
> vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;
> }
>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client
2018-02-05 11:49 ` [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client Daniel P. Berrangé
2018-02-05 13:03 ` Laszlo Ersek
@ 2018-02-05 19:32 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-05 19:32 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Laszlo Ersek, Gerd Hoffmann
[-- Attachment #1: Type: text/plain, Size: 2835 bytes --]
On 02/05/2018 08:49 AM, Daniel P. Berrangé wrote:
> The 'vs->as.freq' value is a signed integer, which is read from an
> unsigned 32-bit int field on the wire. There is thus a risk of overflow
> on 32-bit platforms. Move the frequency limit checking to be done at
> time of read before casting to a signed integer.
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> ui/vnc.c | 22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e14e524764..79ac9eccde 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -982,14 +982,7 @@ static void vnc_update_throttle_offset(VncState *vs)
> vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
>
> if (vs->audio_cap) {
> - int freq = vs->as.freq;
> - /* We don't limit freq when reading settings from client, so
> - * it could be upto MAX_INT in size. 48khz is a sensible
> - * upper bound for trustworthy clients */
> int bps;
> - if (freq > 48000) {
> - freq = 48000;
> - }
> switch (vs->as.fmt) {
> default:
> case AUD_FMT_U8:
> @@ -1005,7 +998,7 @@ static void vnc_update_throttle_offset(VncState *vs)
> bps = 4;
> break;
> }
> - offset += freq * bps * vs->as.nchannels;
> + offset += vs->as.freq * bps * vs->as.nchannels;
> }
>
> /* Put a floor of 1MB on offset, so that if we have a large pending
> @@ -2279,6 +2272,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
> {
> int i;
> uint16_t limit;
> + uint32_t freq;
> VncDisplay *vd = vs->vd;
>
> if (data[0] > 3) {
> @@ -2398,7 +2392,17 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
> vnc_client_error(vs);
> break;
> }
> - vs->as.freq = read_u32(data, 6);
> + freq = read_u32(data, 6);
> + /* No official limit for protocol, but 48khz is a sensible
> + * upper bound for trustworthy clients, and this limit
> + * protects calculations involving 'vs->as.freq' later.
> + */
> + if (freq > 48000) {
> + VNC_DEBUG("Invalid audio frequency %u > 48000", freq);
> + vnc_client_error(vs);
> + break;
> + }
> + vs->as.freq = freq;
> break;
> default:
> VNC_DEBUG("Invalid audio message %d\n", read_u8(data, 4));
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth
2018-02-05 11:49 ` [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth Daniel P. Berrangé
2018-02-05 12:58 ` Laszlo Ersek
@ 2018-02-05 19:32 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-05 19:32 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Laszlo Ersek, Gerd Hoffmann
[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]
On 02/05/2018 08:49 AM, Daniel P. Berrangé wrote:
> The start_auth_sasl() method declares a 'Error *local_err' variable in
> an inner if () {...} scope, which shadows a variable of the same name
> declared at the start of the method. This is confusing for reviewers and
> may trigger compiler warnings.
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> ui/vnc-auth-sasl.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
> index fbccca8c8a..8ebd0d3d00 100644
> --- a/ui/vnc-auth-sasl.c
> +++ b/ui/vnc-auth-sasl.c
> @@ -556,7 +556,6 @@ void start_auth_sasl(VncState *vs)
> /* Inform SASL that we've got an external SSF layer from TLS/x509 */
> if (vs->auth == VNC_AUTH_VENCRYPT &&
> vs->subauth == VNC_AUTH_VENCRYPT_X509SASL) {
> - Error *local_err = NULL;
> int keysize;
> sasl_ssf_t ssf;
>
> @@ -565,7 +564,6 @@ void start_auth_sasl(VncState *vs)
> if (keysize < 0) {
> trace_vnc_auth_fail(vs, vs->auth, "cannot TLS get cipher size",
> error_get_pretty(local_err));
> - error_free(local_err);
> sasl_dispose(&vs->sasl.conn);
> vs->sasl.conn = NULL;
> goto authabort;
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check
2018-02-05 11:49 ` [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check Daniel P. Berrangé
2018-02-05 12:58 ` Laszlo Ersek
@ 2018-02-05 19:32 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-05 19:32 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel; +Cc: Laszlo Ersek, Gerd Hoffmann
[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]
On 02/05/2018 08:49 AM, Daniel P. Berrangé wrote:
> For very large framebuffers, it is theoretically possible for the result
> of 'vs->throttle_output_offset * VNC_THROTTLE_OUTPUT_LIMIT_SCALE' to
> exceed the size of a 32-bit int. For this to happen in practice, the
> video RAM would have to be set to a large enough value, which is not
> likely today. None the less we can be paranoid against future growth by
> using division instead of multiplication when checking the limits.
>
> Reported-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> ui/vnc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 93731accb6..e14e524764 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1572,8 +1572,8 @@ void vnc_write(VncState *vs, const void *data, size_t len)
> * handshake, or from the job thread's VncState clone
> */
> if (vs->throttle_output_offset != 0 &&
> - vs->output.offset > (vs->throttle_output_offset *
> - VNC_THROTTLE_OUTPUT_LIMIT_SCALE)) {
> + (vs->output.offset / VNC_THROTTLE_OUTPUT_LIMIT_SCALE) >
> + vs->throttle_output_offset) {
> trace_vnc_client_output_limit(vs, vs->ioc, vs->output.offset,
> vs->throttle_output_offset);
> vnc_disconnect_start(vs);
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-02-05 19:32 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-05 11:49 [Qemu-devel] [PATCH 0/4] Misc VNC fixes / sanity checks Daniel P. Berrangé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 1/4] ui: avoid risk of 32-bit int overflow in VNC buffer check Daniel P. Berrangé
2018-02-05 12:58 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 2/4] ui: avoid 'local_err' variable shadowing in VNC SASL auth Daniel P. Berrangé
2018-02-05 12:58 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client Daniel P. Berrangé
2018-02-05 13:03 ` Laszlo Ersek
2018-02-05 19:32 ` Philippe Mathieu-Daudé
2018-02-05 11:49 ` [Qemu-devel] [PATCH 4/4] ui: extend VNC trottling tracing to SASL codepaths Daniel P. Berrangé
2018-02-05 13:08 ` Laszlo Ersek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.