[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH 3/4] ui: check VNC audio frequency limit at time of reading from client |
Date: |
Mon, 5 Feb 2018 14:03:14 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 |
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 <address@hidden>
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
> 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 <address@hidden>