[Qemu-devel] [PATCH] qcow2 spec: Describe string header extensions

[Qemu-devel] [PATCH] qcow2 spec: Describe string header extensions

[Kevin Wolf]

Be more specific about the string representation in header extensions.

Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 docs/interop/qcow2.txt | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index bfb97cfde3..8c3098d8d9 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -170,11 +170,11 @@ be stored. Each extension has a structure like the following:
 
     Byte  0 -  3:   Header extension type:
                         0x00000000 - End of the header extension area
-                        0xE2792ACA - Backing file format name
+                        0xE2792ACA - Backing file format name string
                         0x6803f857 - Feature name table
                         0x23852875 - Bitmaps extension
                         0x0537be77 - Full disk encryption header pointer
-                        0x44415441 - External data file name
+                        0x44415441 - External data file name string
                         other      - Unknown header extension, can be safely
                                      ignored
 
@@ -196,6 +196,16 @@ data of compatible features that it doesn't support. Compatible features that
 need space for additional data can use a header extension.
 
 
+== String header extensions ==
+
+Some header extensions (such as the backing file format name and the external
+data file name) are just a single string. In this case, the header extension
+length is the string length and the string is not '\0' terminated. (The header
+extension padding can make it look like a string is '\0' terminated, but
+neither is padding always necessary nor is there a guarantee that zero bytes
+are used for padding.)
+
+
 == Feature name table ==
 
 The feature name table is an optional header extension that contains the name
-- 
2.20.1

Re: [Qemu-devel] [PATCH] qcow2 spec: Describe string header extensions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1588 bytes --]

On 3/7/19 10:53 AM, Kevin Wolf wrote:
> Be more specific about the string representation in header extensions.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  docs/interop/qcow2.txt | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 

Based-on: <20190227172256.30368-1-kwolf@redhat.com>

>  
>  
> +== String header extensions ==
> +
> +Some header extensions (such as the backing file format name and the external
> +data file name) are just a single string. In this case, the header extension
> +length is the string length and the string is not '\0' terminated. (The header
> +extension padding can make it look like a string is '\0' terminated, but
> +neither is padding always necessary nor is there a guarantee that zero bytes
> +are used for padding.)

We didn't require 0 padding?  (goes and re-reads) - oops, yes that's
correct. It makes it harder to extend a struct by making use of that
padding if you can't guarantee what the padding had to be prior to the
extension, and means that you have to consider whether there are any
potential security risks of the padding being used as a side channel to
leak information while still being a well-formed file. But changing the
standard to require zero padding is different than documenting existing
practice, so your patch is correct.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] qcow2 spec: Describe string header extensions

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 403 bytes --]

On Thu, Mar 07, 2019 at 05:53:03PM +0100, Kevin Wolf wrote:
> Be more specific about the string representation in header extensions.
> 
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  docs/interop/qcow2.txt | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] qcow2 spec: Describe string header extensions

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]

Am 07.03.2019 um 18:40 hat Eric Blake geschrieben:
> On 3/7/19 10:53 AM, Kevin Wolf wrote:
> > Be more specific about the string representation in header extensions.
> > 
> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  docs/interop/qcow2.txt | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> > 
> 
> Based-on: <20190227172256.30368-1-kwolf@redhat.com>
> 
> >  
> >  
> > +== String header extensions ==
> > +
> > +Some header extensions (such as the backing file format name and the external
> > +data file name) are just a single string. In this case, the header extension
> > +length is the string length and the string is not '\0' terminated. (The header
> > +extension padding can make it look like a string is '\0' terminated, but
> > +neither is padding always necessary nor is there a guarantee that zero bytes
> > +are used for padding.)
> 
> We didn't require 0 padding?  (goes and re-reads) - oops, yes that's
> correct.

Yes. QEMU does use zeroes, but the spec doesn't mandate it and changing
it would be an incompatible change. And the worst that could happen is
that someone leaves a hidden message in the padding, which doesn't
really hurt.

> It makes it harder to extend a struct by making use of that
> padding if you can't guarantee what the padding had to be prior to the
> extension, and means that you have to consider whether there are any
> potential security risks of the padding being used as a side channel to
> leak information while still being a well-formed file.

It doesn't actually make it harder because we have the length field
which tells us the exact length of the valid data, so we shouldn't even
try to use any of the padding.

> But changing the standard to require zero padding is different than
> documenting existing practice, so your patch is correct.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]