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

Re: [libvirt] [PATCH 01/11] Extract the backing store format as well as name, if available



On Mon, Jul 12, 2010 at 02:30:38PM +0100, Daniel P. Berrange wrote:
> When QEMU opens a backing store for a QCow2 file, it will
> normally auto-probe for the format of the backing store,
> rather than assuming it has the same format as the referencing
> file. There is a QCow2 extension that allows an explicit format
> for the backing store to be embedded in the referencing file.
> This closes the auto-probing security hole in QEMU.
> 
> This backing store format can be useful for libvirt users
> of virStorageFileGetMetadata, so extract this data and report
> it.
> 
> QEMU does not require disk image backing store files to be in
> the same format the file linkee. It will auto-probe the disk
> format for the backing store when opening it. If the backing
> store was intended to be a raw file this could be a security
> hole, because a guest may have written data into its disk that
> then makes the backing store look like a qcow2 file. If it can
> trick QEMU into thinking the raw file is a qcow2 file, it can
> access arbitrary files on the host by adding further backing
> store links.
> 
> To address this, callers of virStorageFileGetMeta need to be
> told of the backing store format. If no format is declared,
> they can make a decision whether to allow format probing or
> not.
> ---
>  src/util/storage_file.c |  206 +++++++++++++++++++++++++++++++++++++++++------
>  src/util/storage_file.h |    2 +
>  2 files changed, 183 insertions(+), 25 deletions(-)
> 
> diff --git a/src/util/storage_file.c b/src/util/storage_file.c
> index 0adea40..80f743e 100644
> --- a/src/util/storage_file.c
> +++ b/src/util/storage_file.c
> @@ -78,12 +78,33 @@ struct FileTypeInfo {
>      int qcowCryptOffset;  /* Byte offset from start of file
>                             * where to find encryption mode,
>                             * -1 if encryption is not used */
> -    int (*getBackingStore)(char **res, const unsigned char *buf, size_t buf_size);
> +    int (*getBackingStore)(char **res, int *format,
> +                           const unsigned char *buf, size_t buf_size);
>  };
>  
> -static int cowGetBackingStore(char **, const unsigned char *, size_t);
> -static int qcowXGetBackingStore(char **, const unsigned char *, size_t);
> -static int vmdk4GetBackingStore(char **, const unsigned char *, size_t);
> +static int cowGetBackingStore(char **, int *,
> +                              const unsigned char *, size_t);
> +static int qcow1GetBackingStore(char **, int *,
> +                                const unsigned char *, size_t);
> +static int qcow2GetBackingStore(char **, int *,
> +                                const unsigned char *, size_t);
> +static int vmdk4GetBackingStore(char **, int *,
> +                                const unsigned char *, size_t);
> +
> +#define QCOWX_HDR_VERSION (4)
> +#define QCOWX_HDR_BACKING_FILE_OFFSET (QCOWX_HDR_VERSION+4)
> +#define QCOWX_HDR_BACKING_FILE_SIZE (QCOWX_HDR_BACKING_FILE_OFFSET+8)
> +#define QCOWX_HDR_IMAGE_SIZE (QCOWX_HDR_BACKING_FILE_SIZE+4+4)
> +
> +#define QCOW1_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8+1+1)
> +#define QCOW2_HDR_CRYPT (QCOWX_HDR_IMAGE_SIZE+8)
> +
> +#define QCOW1_HDR_TOTAL_SIZE (QCOW1_HDR_CRYPT+4+8)
> +#define QCOW2_HDR_TOTAL_SIZE (QCOW2_HDR_CRYPT+4+4+8+8+4+4+8)
> +
> +#define QCOW2_HDR_EXTENSION_END 0
> +#define QCOW2_HDR_EXTENSION_BACKING_FORMAT 0xE2792ACA
> +
>  
>  
>  static struct FileTypeInfo const fileTypeInfo[] = {
> @@ -119,11 +140,11 @@ static struct FileTypeInfo const fileTypeInfo[] = {
>      /* QCow */
>      { VIR_STORAGE_FILE_QCOW, "QFI", NULL,
>        LV_BIG_ENDIAN, 4, 1,
> -      4+4+8+4+4, 8, 1, 4+4+8+4+4+8+1+1+2, qcowXGetBackingStore },
> +      QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW1_HDR_CRYPT, qcow1GetBackingStore },
>      /* QCow 2 */
>      { VIR_STORAGE_FILE_QCOW2, "QFI", NULL,
>        LV_BIG_ENDIAN, 4, 2,
> -      4+4+8+4+4, 8, 1, 4+4+8+4+4+8, qcowXGetBackingStore },
> +      QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore },
>      /* VMDK 3 */
>      /* XXX Untested
>      { VIR_STORAGE_FILE_VMDK, "COWD", NULL,
> @@ -142,11 +163,14 @@ static struct FileTypeInfo const fileTypeInfo[] = {
>  
>  static int
>  cowGetBackingStore(char **res,
> +                   int *format,
>                     const unsigned char *buf,
>                     size_t buf_size)
>  {
>  #define COW_FILENAME_MAXLEN 1024
>      *res = NULL;
> +    *format = VIR_STORAGE_FILE_AUTO;
> +
>      if (buf_size < 4+4+ COW_FILENAME_MAXLEN)
>          return BACKING_STORE_INVALID;
>      if (buf[4+4] == '\0') /* cow_header_v2.backing_file[0] */
> @@ -160,31 +184,98 @@ cowGetBackingStore(char **res,
>      return BACKING_STORE_OK;
>  }
>  
> +
> +static int
> +qcow2GetBackingStoreFormat(int *format,
> +                           const unsigned char *buf,
> +                           size_t buf_size,
> +                           size_t extension_start,
> +                           size_t extension_end)
> +{
> +    size_t offset = extension_start;
> +
> +    /*
> +     * The extensions take format of
> +     *
> +     * int32: magic
> +     * int32: length
> +     * byte[length]: payload
> +     *
> +     * Unknown extensions can be ignored by skipping
> +     * over "length" bytes in the data stream.
> +     */
> +    while (offset < (buf_size-8) &&
> +           offset < (extension_end-8)) {
> +        unsigned int magic =
> +            (buf[offset] << 24) +
> +            (buf[offset+1] << 16) +
> +            (buf[offset+2] << 8) +
> +            (buf[offset+3]);
> +        unsigned int len =
> +            (buf[offset+4] << 24) +
> +            (buf[offset+5] << 16) +
> +            (buf[offset+6] << 8) +
> +            (buf[offset+7]);
> +
> +        offset += 8;
> +
> +        if ((offset + len) < offset)
> +            break;
> +
> +        if ((offset + len) > buf_size)
> +            break;
> +
> +        switch (magic) {
> +        case QCOW2_HDR_EXTENSION_END:
> +            goto done;
> +
> +        case QCOW2_HDR_EXTENSION_BACKING_FORMAT:
> +            if (buf[offset+len] != '\0')
> +                break;
> +            *format = virStorageFileFormatTypeFromString(
> +                ((const char *)buf)+offset);
> +            break;
> +        }
> +
> +        offset += len;
> +    }
> +
> +done:
> +
> +    return 0;
> +}
> +
> +
>  static int
>  qcowXGetBackingStore(char **res,
> +                     int *format,
>                       const unsigned char *buf,
> -                     size_t buf_size)
> +                     size_t buf_size,
> +                     bool isQCow2)
>  {
>      unsigned long long offset;
>      unsigned long size;
>  
>      *res = NULL;
> -    if (buf_size < 4+4+8+4)
> +    if (format)
> +        *format = VIR_STORAGE_FILE_AUTO;
> +
> +    if (buf_size < QCOWX_HDR_BACKING_FILE_OFFSET+8+4)
>          return BACKING_STORE_INVALID;
> -    offset = (((unsigned long long)buf[4+4] << 56)
> -              | ((unsigned long long)buf[4+4+1] << 48)
> -              | ((unsigned long long)buf[4+4+2] << 40)
> -              | ((unsigned long long)buf[4+4+3] << 32)
> -              | ((unsigned long long)buf[4+4+4] << 24)
> -              | ((unsigned long long)buf[4+4+5] << 16)
> -              | ((unsigned long long)buf[4+4+6] << 8)
> -              | buf[4+4+7]); /* QCowHeader.backing_file_offset */
> +    offset = (((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET] << 56)
> +              | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+1] << 48)
> +              | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+2] << 40)
> +              | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+3] << 32)
> +              | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+4] << 24)
> +              | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+5] << 16)
> +              | ((unsigned long long)buf[QCOWX_HDR_BACKING_FILE_OFFSET+6] << 8)
> +              | buf[QCOWX_HDR_BACKING_FILE_OFFSET+7]); /* QCowHeader.backing_file_offset */
>      if (offset > buf_size)
>          return BACKING_STORE_INVALID;
> -    size = ((buf[4+4+8] << 24)
> -            | (buf[4+4+8+1] << 16)
> -            | (buf[4+4+8+2] << 8)
> -            | buf[4+4+8+3]); /* QCowHeader.backing_file_size */
> +    size = ((buf[QCOWX_HDR_BACKING_FILE_SIZE] << 24)
> +            | (buf[QCOWX_HDR_BACKING_FILE_SIZE+1] << 16)
> +            | (buf[QCOWX_HDR_BACKING_FILE_SIZE+2] << 8)
> +            | buf[QCOWX_HDR_BACKING_FILE_SIZE+3]); /* QCowHeader.backing_file_size */
>      if (size == 0)
>          return BACKING_STORE_OK;
>      if (offset + size > buf_size || offset + size < offset)
> @@ -197,12 +288,63 @@ qcowXGetBackingStore(char **res,
>      }
>      memcpy(*res, buf + offset, size);
>      (*res)[size] = '\0';
> +
> +    /*
> +     * Traditionally QCow2 files had a layout of
> +     *
> +     * [header]
> +     * [backingStoreName]
> +     *
> +     * Although the backingStoreName typically followed
> +     * the header immediately, this was not required by
> +     * the format. By specifying a higher byte offset for
> +     * the backing file offset in the header, it was
> +     * possible to leave space between the header and
> +     * start of backingStore.
> +     *
> +     * This hack is now used to store extensions to the
> +     * qcow2 format:
> +     *
> +     * [header]
> +     * [extensions]
> +     * [backingStoreName]
> +     *
> +     * Thus the file region to search for extensions is
> +     * between the end of the header (QCOW2_HDR_TOTAL_SIZE)
> +     * and the start of the backingStoreName (offset)
> +     */
> +    if (isQCow2)
> +        qcow2GetBackingStoreFormat(format, buf, buf_size, QCOW2_HDR_TOTAL_SIZE, offset);
> +
>      return BACKING_STORE_OK;
>  }
>  
>  
>  static int
> +qcow1GetBackingStore(char **res,
> +                     int *format,
> +                     const unsigned char *buf,
> +                     size_t buf_size)
> +{
> +    /* QCow1 doesn't have the extensions capability
> +     * used to store backing format */
> +    *format = VIR_STORAGE_FILE_AUTO;
> +    return qcowXGetBackingStore(res, NULL, buf, buf_size, false);
> +}
> +
> +static int
> +qcow2GetBackingStore(char **res,
> +                     int *format,
> +                     const unsigned char *buf,
> +                     size_t buf_size)
> +{
> +    return qcowXGetBackingStore(res, format, buf, buf_size, true);
> +}
> +
> +
> +static int
>  vmdk4GetBackingStore(char **res,
> +                     int *format,
>                       const unsigned char *buf,
>                       size_t buf_size)
>  {
> @@ -212,6 +354,14 @@ vmdk4GetBackingStore(char **res,
>      size_t len;
>  
>      *res = NULL;
> +    /*
> +     * Technically this should have been VMDK, since
> +     * VMDK spec / VMWare impl only support VMDK backed
> +     * by VMDK. QEMU isn't following this though and
> +     * does probing on VMDK backing files, hence we set
> +     * AUTO
> +     */
> +    *format = VIR_STORAGE_FILE_AUTO;
>  
>      if (buf_size <= 0x200)
>          return BACKING_STORE_INVALID;
> @@ -358,9 +508,12 @@ virStorageFileGetMetadataFromFD(const char *path,
>          /* Validation passed, we know the file format now */
>          meta->format = fileTypeInfo[i].type;
>          if (fileTypeInfo[i].getBackingStore != NULL) {
> -            char *base;
> +            char *backing;
> +            int backingFormat;
>  
> -            switch (fileTypeInfo[i].getBackingStore(&base, head, len)) {
> +            switch (fileTypeInfo[i].getBackingStore(&backing,
> +                                                    &backingFormat,
> +                                                    head, len)) {
>              case BACKING_STORE_OK:
>                  break;
>  
> @@ -370,13 +523,16 @@ virStorageFileGetMetadataFromFD(const char *path,
>              case BACKING_STORE_ERROR:
>                  return -1;
>              }
> -            if (base != NULL) {
> -                meta->backingStore = absolutePathFromBaseFile(path, base);
> -                VIR_FREE(base);
> +            if (backing != NULL) {
> +                meta->backingStore = absolutePathFromBaseFile(path, backing);
> +                VIR_FREE(backing);
>                  if (meta->backingStore == NULL) {
>                      virReportOOMError();
>                      return -1;
>                  }
> +                meta->backingStoreFormat = backingFormat;
> +            } else {
> +                meta->backingStoreFormat = VIR_STORAGE_FILE_AUTO;
>              }
>          }
>          return 0;
> diff --git a/src/util/storage_file.h b/src/util/storage_file.h
> index 58533ee..6328ba7 100644
> --- a/src/util/storage_file.h
> +++ b/src/util/storage_file.h
> @@ -28,6 +28,7 @@
>  # include <stdbool.h>
>  
>  enum virStorageFileFormat {
> +    VIR_STORAGE_FILE_AUTO = -1,
>      VIR_STORAGE_FILE_RAW = 0,
>      VIR_STORAGE_FILE_DIR,
>      VIR_STORAGE_FILE_BOCHS,
> @@ -47,6 +48,7 @@ VIR_ENUM_DECL(virStorageFileFormat);
>  typedef struct _virStorageFileMetadata {
>      int format;
>      char *backingStore;
> +    int backingStoreFormat;
>      unsigned long long capacity;
>      bool encrypted;
>  } virStorageFileMetadata;

  ACK, also includes some cleanup about the indexes in the qcow formats

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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