All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.