Re: [Qemu-devel] [PATCH] qga: check bytes count read by guest-file-read

[Daniel P . Berrangé]

On Wed, Jun 13, 2018 at 11:46:57AM +0530, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> While reading file content via 'guest-file-read' command,
> 'qmp_guest_file_read' routine allocates buffer of count+1
> bytes. It could overflow for large values of 'count'.
> Add check to avoid it.

No objection to this patch, but I would point out that even
trying to read  'UINT32_MAX - 1' bytes is going to end in
disaster. 

We'll allocate UINT32_MAX bytes of RAM to read the data.

Then we'll allocate

   (UINT32_MAX / 3 + 1) * 4 + 1)

bytes of RAM in g_base64_encode.... which incidentally
is not checking for integer overflow either when calling
g_malloc.

Then our JSON formatting code will allocate at least that
much RAM again, probably also not checking for overflow.

I wouldn't be surprised if we allocate that much RAM yet
again in some other part of the stack too.

> 
> Reported-by: Fakhri Zulkifli <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  qga/commands-posix.c | 2 +-
>  qga/commands-win32.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index eae817191b..068c0f0bd9 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -458,7 +458,7 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, 
> bool has_count,
>  
>      if (!has_count) {
>          count = QGA_READ_COUNT_DEFAULT;
> -    } else if (count < 0) {
> +    } else if (count < 0 || count >= UINT32_MAX) {
>          error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
>                     count);
>          return NULL;
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 70ee5379f6..73f31fa8c2 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -318,7 +318,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool 
> has_count,
>      }
>      if (!has_count) {
>          count = QGA_READ_COUNT_DEFAULT;
> -    } else if (count < 0) {
> +    } else if (count < 0 || count >= UINT32_MAX) {
>          error_setg(errp, "value '%" PRId64
>                     "' is invalid for argument count", count);
>          return NULL;
> -- 
> 2.17.1
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|