* [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation
@ 2014-09-16 19:12 Jeff Cody
2014-09-17 6:33 ` Markus Armbruster
2014-09-19 9:45 ` Stefan Hajnoczi
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Cody @ 2014-09-16 19:12 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, stefanha
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 <jcody@redhat.com>
---
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))
/*
* 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;
}
@@ -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:
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation
2014-09-16 19:12 [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation Jeff Cody
@ 2014-09-17 6:33 ` Markus Armbruster
2014-09-17 19:54 ` Jeff Cody
2014-09-19 9:45 ` Stefan Hajnoczi
1 sibling, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2014-09-17 6:33 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, qemu-devel, stefanha
Jeff Cody <jcody@redhat.com> 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 <jcody@redhat.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation
2014-09-17 6:33 ` Markus Armbruster
@ 2014-09-17 19:54 ` Jeff Cody
2014-09-18 6:47 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Cody @ 2014-09-17 19:54 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, stefanha
On Wed, Sep 17, 2014 at 08:33:10AM +0200, Markus Armbruster wrote:
> Jeff Cody <jcody@redhat.com> 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 <jcody@redhat.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation
2014-09-17 19:54 ` Jeff Cody
@ 2014-09-18 6:47 ` Markus Armbruster
2014-09-18 7:43 ` Markus Armbruster
0 siblings, 1 reply; 6+ messages in thread
From: Markus Armbruster @ 2014-09-18 6:47 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, qemu-devel, stefanha
Jeff Cody <jcody@redhat.com> writes:
> On Wed, Sep 17, 2014 at 08:33:10AM +0200, Markus Armbruster wrote:
>> Jeff Cody <jcody@redhat.com> 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 <jcody@redhat.com>
>> > ---
>> > 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?
I didn't count anything, I trusted my eyes, which screamed "ugly!" :)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation
2014-09-18 6:47 ` Markus Armbruster
@ 2014-09-18 7:43 ` Markus Armbruster
0 siblings, 0 replies; 6+ messages in thread
From: Markus Armbruster @ 2014-09-18 7:43 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, qemu-devel, stefanha
Markus Armbruster <armbru@redhat.com> writes:
> Jeff Cody <jcody@redhat.com> writes:
>
>> On Wed, Sep 17, 2014 at 08:33:10AM +0200, Markus Armbruster wrote:
>>> Jeff Cody <jcody@redhat.com> 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 <jcody@redhat.com>
>>> > ---
>>> > 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?
>
> I didn't count anything, I trusted my eyes, which screamed "ugly!" :)
Forgot to say: since checkpatch is happy, I guess this is a technically
a matter of taste, so
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Just in case you actually *like* hanging your code right off the right
margin of the window. *Shudder* ;-P
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation
2014-09-16 19:12 [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation Jeff Cody
2014-09-17 6:33 ` Markus Armbruster
@ 2014-09-19 9:45 ` Stefan Hajnoczi
1 sibling, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2014-09-19 9:45 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 519 bytes --]
On Tue, Sep 16, 2014 at 03:12:06PM -0400, Jeff Cody wrote:
> 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 <jcody@redhat.com>
> ---
> block/vhdx.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-19 9:45 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-16 19:12 [Qemu-devel] [PATCH] block: vhdx - fix reading beyond pointer during image creation Jeff Cody
2014-09-17 6:33 ` Markus Armbruster
2014-09-17 19:54 ` Jeff Cody
2014-09-18 6:47 ` Markus Armbruster
2014-09-18 7:43 ` Markus Armbruster
2014-09-19 9:45 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).