Message-ID: <535E1FDE.50209@msgid.tls.msk.ru>
Date: Mon, 28 Apr 2014 13:31:10 +0400
From: Michael Tokarev <mjt@tls.msk.ru>
Organization: Telecom Service, JSC
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
	rv:24.0) Gecko/20100101 Icedove/24.4.0
MIME-Version: 1.0
To: Deepak Kathayat <deepak.mk17@gmail.com>, qemu-devel@nongnu.org
References: <1395217173-20902-1-git-send-email-deepak.mk17@gmail.com>
In-Reply-To: <1395217173-20902-1-git-send-email-deepak.mk17@gmail.com>
OpenPGP: id=804465C5
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Cc: qemu-trivial@nongnu.org, stefanha@redhat.com
Subject: Re: [Qemu-trivial] [PATCH] qcow2.c: Check if backing file name
 length is valid
Precedence: list

19.03.2014 12:19, Deepak Kathayat wrote:
> Signed-off-by: Deepak Kathayat <deepak.mk17@gmail.com>
> ---
> The len variable is a signed integer whereas the backing file name
>  length in the image header is unsigned. Therefore, it may
>  overflow. Furthermore, backing file name length cannot be
>  zero. These two cases must be handled explicitly.
>  block/qcow2.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 945c9d6..7b6f65c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -625,6 +625,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      /* read the backing file name */
>      if (header.backing_file_offset != 0) {
>          len = header.backing_file_size;
> +        if (len <= 0) {
> +            error_setg(errp, "Invalid backing file name length: %d", len);
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>          if (len > 1023) {
>              len = 1023;
>          }

A better fix has been implemented meanwhile, as a part of input format validation
series:

commit 6d33e8e7dc9d40ea105feed4b39caa3e641569e8
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Wed Mar 26 13:05:47 2014 +0100

    qcow2: Fix backing file name length check

    len could become negative and would pass the check then. Nothing bad
    happened because bdrv_pread() happens to return an error for negative
    length values, but make variables for sizes unsigned anyway.

    This patch also changes the behaviour to error out on invalid lengths
    instead of silently truncating it to 1023.


Thanks,

/mjt