[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] util: storage: Fix qcow(2) header parser according to docs




On 09/15/2014 10:22 AM, Peter Krempa wrote:
> The backing store string location offset 0 determines that the file
> isn't present. The string size shouldn't be then checked:
> 
> from qemu.git/docs/specs/qcow2.txt
> 
> == Header ==
> 
> The first cluster of a qcow2 image contains the file header:
> 
> Byte  0 -  3:   magic
>                 QCOW magic string ("QFI\xfb")
> 
>       4 -  7:   version
>                 Version number (valid values are 2 and 3)
> 
>       8 - 15:   backing_file_offset
>                 Offset into the image file at which the backing file name
>                 is stored (NB: The string is not null terminated). 0 if the
>                 image doesn't have a backing file.
> 
>      16 - 19:   backing_file_size
>                 Length of the backing file name in bytes. Must not be
>                 longer than 1023 bytes. Undefined if the image doesn't have
>                 a backing file.         ^^^^^^^^^
> 
> This patch intentionally leaves the backing file string size check in
> place in case a malformatted file would be presented to libvirt. Also
> according to the docs the string size is maximum 1023 bytes, thus this
> patch adds a check to verify that.
> 
> I was also able to verify that the check was done the same way in the
> legacy qcow fromat (in qemu's code).
> ---
>  src/util/virstoragefile.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index 5db9184..13056a7 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -385,15 +385,22 @@ qcowXGetBackingStore(char **res,
>      offset = virReadBufInt64BE(buf + QCOWX_HDR_BACKING_FILE_OFFSET);
>      if (offset > buf_size)
>          return BACKING_STORE_INVALID;
> +
> +    if (offset == 0) {
> +        if (format)
> +            *format = VIR_STORAGE_FILE_NONE;
> +        return BACKING_STORE_OK;
> +    }
> +
>      size = virReadBufInt32BE(buf + QCOWX_HDR_BACKING_FILE_SIZE);
>      if (size == 0) {
>          if (format)
>              *format = VIR_STORAGE_FILE_NONE;
>          return BACKING_STORE_OK;
>      }
> -    if (offset + size > buf_size || offset + size < offset)
> +    if (size > 1023)
>          return BACKING_STORE_INVALID;
> -    if (size + 1 == 0)
> +    if (offset + size > buf_size || offset + size < offset)
>          return BACKING_STORE_INVALID;
>      if (VIR_ALLOC_N(*res, size + 1) < 0)
>          return BACKING_STORE_ERROR;
> 

ACK

And yes, Coverity is fine with this - I will remove patch 3 from my
other series before pushing...

Tks,

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]