Re: [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation

[Markus Armbruster]

Jeff Cody <address@hidden> writes:

> In vhdx_create_metadata(), we allocate 40 bytes to entry_buffer for
> the various metadata table entries.  However, we write out 64kB from
> that buffer into the new file.  Only write out the correct 40 bytes.
>
> Signed-off-by: Jeff Cody <address@hidden>
> ---
>  block/vhdx.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/block/vhdx.c b/block/vhdx.c
> index 796b7bd..b52ec32 100644
> --- a/block/vhdx.c
> +++ b/block/vhdx.c
> @@ -1407,6 +1407,12 @@ exit:
>      return ret;
>  }
>  
> +#define VHDX_METADATA_ENTRY_BUFFER_SIZE \
> +                                    (sizeof(VHDXFileParameters)              
>  +\
> +                                     sizeof(VHDXVirtualDiskSize)             
>  +\
> +                                     sizeof(VHDXPage83Data)                  
>  +\
> +                                     
> sizeof(VHDXVirtualDiskLogicalSectorSize) +\
> +                                     
> sizeof(VHDXVirtualDiskPhysicalSectorSize))

Long lines, caused by excessive indentation.  Emacs suggests

#define VHDX_METADATA_ENTRY_BUFFER_SIZE         \
    (sizeof(VHDXFileParameters)               + \
     sizeof(VHDXVirtualDiskSize)              + \
     sizeof(VHDXPage83Data)                   + \
     sizeof(VHDXVirtualDiskLogicalSectorSize) + \
     sizeof(VHDXVirtualDiskPhysicalSectorSize))

>  
>  /*
>   * Create the Metadata entries.
> @@ -1445,11 +1451,7 @@ static int vhdx_create_new_metadata(BlockDriverState 
> *bs,
>      VHDXVirtualDiskLogicalSectorSize  *mt_log_sector_size;
>      VHDXVirtualDiskPhysicalSectorSize *mt_phys_sector_size;
>  
> -    entry_buffer = g_malloc0(sizeof(VHDXFileParameters)               +
> -                             sizeof(VHDXVirtualDiskSize)              +
> -                             sizeof(VHDXPage83Data)                   +
> -                             sizeof(VHDXVirtualDiskLogicalSectorSize) +
> -                             sizeof(VHDXVirtualDiskPhysicalSectorSize));
> +    entry_buffer = g_malloc0(VHDX_METADATA_ENTRY_BUFFER_SIZE);
>  
>      mt_file_params = entry_buffer;
>      offset += sizeof(VHDXFileParameters);
> @@ -1530,7 +1532,7 @@ static int vhdx_create_new_metadata(BlockDriverState 
> *bs,
>      }
>  
>      ret = bdrv_pwrite(bs, metadata_offset + (64 * KiB), entry_buffer,
> -                      VHDX_HEADER_BLOCK_SIZE);
> +                      VHDX_METADATA_ENTRY_BUFFER_SIZE);
>      if (ret < 0) {
>          goto exit;
>      }

Fixes read beyond end of buffer.  Crash bug when sufficiently unlucky.

> @@ -1725,7 +1727,6 @@ static int 
> vhdx_create_new_region_table(BlockDriverState *bs,
>          goto exit;
>      }
>  
> -
>  exit:
>      g_free(s);
>      g_free(buffer);
> @@ -1876,7 +1877,6 @@ static int vhdx_create(const char *filename, QemuOpts 
> *opts, Error **errp)
>      }
>  
>  
> -
>  delete_and_exit:
>      bdrv_unref(bs);
>  exit:

These two hunks are unrelated.  I wouldn't include them.  Advice, not
objection.

Keeping two out of three blank lines looks odd, but pairs of blank lines
occur elsewhere in the function, so I guess it's intentional.

If you clean up the long lines, you can add my R-by.