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

[Jeff Cody]

On Wed, Sep 17, 2014 at 08:33:10AM +0200, Markus Armbruster wrote:
> 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))
> 

So, I was getting ready to respin this, but double checked the patch -
it shows the lines ending on column 80 (as intended), and
checkpatch.pl had no issue with it.  Did you accidentally (or
intentionally!) count the leading '+' of the patch itself?

> >  
> >  /*
> >   * 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.