[Qemu-devel] [PULL 0/1] device_tree: load_device_tree(): Allow NULL sizep

[Qemu-devel] [PULL 0/1] device_tree: load_device_tree(): Allow NULL sizep

[Peter A. G. Crosthwaite]

The following changes since commit 3d1d9652978ac5a32a0beb4bdf6065ca39440d89:
  Bruce Rogers (1):
        handle device help before accelerator set up

are available in the git repository at:

  git://developer.petalogix.com/public/qemu.git for-upstream/device-tree-null-size.next

Peter A. G. Crosthwaite (1):
      device_tree: load_device_tree(): Allow NULL sizep

 device_tree.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

[Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep

[Peter A. G. Crosthwaite]

The sizep arg is populated with the size of the loaded device tree. Since this
is one of those informational "please populate" type arguments it should be
optional. Guarded writes to *sizep against NULL accordingly.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
Acked-by: Alexander Graf <agraf@suse.de>
---
 device_tree.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/device_tree.c b/device_tree.c
index d7a9b6b..641a48a 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -71,7 +71,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
     int ret;
     void *fdt = NULL;
 
-    *sizep = 0;
+    if (sizep) {
+        *sizep = 0;
+    }
     dt_size = get_image_size(filename_path);
     if (dt_size < 0) {
         printf("Unable to get size of device tree file '%s'\n",
@@ -104,7 +106,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
             filename_path);
         goto fail;
     }
-    *sizep = dt_size;
+    if (sizep) {
+        *sizep = dt_size;
+    }
     return fdt;
 
 fail:
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep

[Stefan Hajnoczi]

On Fri, Aug 10, 2012 at 01:54:26PM +1000, Peter A. G. Crosthwaite wrote:
> The sizep arg is populated with the size of the loaded device tree. Since this
> is one of those informational "please populate" type arguments it should be
> optional. Guarded writes to *sizep against NULL accordingly.
> 
> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> Acked-by: Alexander Graf <agraf@suse.de>
> ---
>  device_tree.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/device_tree.c b/device_tree.c
> index d7a9b6b..641a48a 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -71,7 +71,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>      int ret;
>      void *fdt = NULL;
>  
> -    *sizep = 0;
> +    if (sizep) {
> +        *sizep = 0;
> +    }
>      dt_size = get_image_size(filename_path);
>      if (dt_size < 0) {
>          printf("Unable to get size of device tree file '%s'\n",
> @@ -104,7 +106,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>              filename_path);
>          goto fail;
>      }
> -    *sizep = dt_size;
> +    if (sizep) {
> +        *sizep = dt_size;
> +    }

What can the caller do with this void* buffer without knowing its size?

They cannot hand the buffer to the guest unless they duplicate the
get_image_size() call, which is pointless, or we're assuming a fixed
size, which is unsafe.  I'm asking what the legitimate use case for
sizep == NULL is?

Stefan

Re: [Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep

[Peter Crosthwaite]

On Fri, Aug 10, 2012 at 11:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Aug 10, 2012 at 01:54:26PM +1000, Peter A. G. Crosthwaite wrote:
>> The sizep arg is populated with the size of the loaded device tree. Since this
>> is one of those informational "please populate" type arguments it should be
>> optional. Guarded writes to *sizep against NULL accordingly.
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> Acked-by: Alexander Graf <agraf@suse.de>
>> ---
>>  device_tree.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/device_tree.c b/device_tree.c
>> index d7a9b6b..641a48a 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -71,7 +71,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>      int ret;
>>      void *fdt = NULL;
>>
>> -    *sizep = 0;
>> +    if (sizep) {
>> +        *sizep = 0;
>> +    }
>>      dt_size = get_image_size(filename_path);
>>      if (dt_size < 0) {
>>          printf("Unable to get size of device tree file '%s'\n",
>> @@ -104,7 +106,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>              filename_path);
>>          goto fail;
>>      }
>> -    *sizep = dt_size;
>> +    if (sizep) {
>> +        *sizep = dt_size;
>> +    }
>
> What can the caller do with this void* buffer without knowing its size?
>

Sanity check the machine:

dtb = load_device_tree( ... ); //dont care how big it is
foo = fdt_gep_prop( dtb, ... );
if (foo != object_get_prop(foo_device, foo_prop, ... )) {
    hw_error("your dtb is bad because ... !\n", ... );
}
...
boot();

This happens at machine init, which is quite separate from boot.

> They cannot hand the buffer to the guest unless they duplicate the
> get_image_size() call, which is pointless, or we're assuming a fixed
> size, which is unsafe.  I'm asking what the legitimate use case for
> sizep == NULL is?

Sanity check the dtb without passing it to the guest. My CAD tools
emit a DTB for the hardware, but it wont always go through a linux
bootloader (E.G. I may want to boot random elfs). I still will pass
the CAD generated DTB to QEMU for sanity check however so qemu can
give me a nice report of what is and isn't modelled.

Regards,
Peter

>
> Stefan

Re: [Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep

[Stefan Hajnoczi]

On Sat, Aug 11, 2012 at 12:11 AM, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> On Fri, Aug 10, 2012 at 11:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Fri, Aug 10, 2012 at 01:54:26PM +1000, Peter A. G. Crosthwaite wrote:
>>> The sizep arg is populated with the size of the loaded device tree. Since this
>>> is one of those informational "please populate" type arguments it should be
>>> optional. Guarded writes to *sizep against NULL accordingly.
>>>
>>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>>> Acked-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>  device_tree.c |    8 ++++++--
>>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/device_tree.c b/device_tree.c
>>> index d7a9b6b..641a48a 100644
>>> --- a/device_tree.c
>>> +++ b/device_tree.c
>>> @@ -71,7 +71,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>>      int ret;
>>>      void *fdt = NULL;
>>>
>>> -    *sizep = 0;
>>> +    if (sizep) {
>>> +        *sizep = 0;
>>> +    }
>>>      dt_size = get_image_size(filename_path);
>>>      if (dt_size < 0) {
>>>          printf("Unable to get size of device tree file '%s'\n",
>>> @@ -104,7 +106,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>>              filename_path);
>>>          goto fail;
>>>      }
>>> -    *sizep = dt_size;
>>> +    if (sizep) {
>>> +        *sizep = dt_size;
>>> +    }
>>
>> What can the caller do with this void* buffer without knowing its size?
>>
>
> Sanity check the machine:
>
> dtb = load_device_tree( ... ); //dont care how big it is
> foo = fdt_gep_prop( dtb, ... );
> if (foo != object_get_prop(foo_device, foo_prop, ... )) {
>     hw_error("your dtb is bad because ... !\n", ... );
> }

What happens if the fdt is corrupt or malicious?  I guess we'll access
memory beyond the end of blob.

This seems to be libfdt's fault.  I didn't see an API to validate the
blob's size.

I'm "happy" with this patch but if fdt's can ever come from untrusted
sources then we're in trouble.

Stefan

Re: [Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep

[Stefan Hajnoczi]

On Sat, Aug 11, 2012 at 01:33:42PM +0100, Stefan Hajnoczi wrote:
> On Sat, Aug 11, 2012 at 12:11 AM, Peter Crosthwaite
> <peter.crosthwaite@petalogix.com> wrote:
> > On Fri, Aug 10, 2012 at 11:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> On Fri, Aug 10, 2012 at 01:54:26PM +1000, Peter A. G. Crosthwaite wrote:
> >>> The sizep arg is populated with the size of the loaded device tree. Since this
> >>> is one of those informational "please populate" type arguments it should be
> >>> optional. Guarded writes to *sizep against NULL accordingly.
> >>>
> >>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> >>> Acked-by: Alexander Graf <agraf@suse.de>
> >>> ---
> >>>  device_tree.c |    8 ++++++--
> >>>  1 files changed, 6 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/device_tree.c b/device_tree.c
> >>> index d7a9b6b..641a48a 100644
> >>> --- a/device_tree.c
> >>> +++ b/device_tree.c
> >>> @@ -71,7 +71,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
> >>>      int ret;
> >>>      void *fdt = NULL;
> >>>
> >>> -    *sizep = 0;
> >>> +    if (sizep) {
> >>> +        *sizep = 0;
> >>> +    }
> >>>      dt_size = get_image_size(filename_path);
> >>>      if (dt_size < 0) {
> >>>          printf("Unable to get size of device tree file '%s'\n",
> >>> @@ -104,7 +106,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
> >>>              filename_path);
> >>>          goto fail;
> >>>      }
> >>> -    *sizep = dt_size;
> >>> +    if (sizep) {
> >>> +        *sizep = dt_size;
> >>> +    }
> >>
> >> What can the caller do with this void* buffer without knowing its size?
> >>
> >
> > Sanity check the machine:
> >
> > dtb = load_device_tree( ... ); //dont care how big it is
> > foo = fdt_gep_prop( dtb, ... );
> > if (foo != object_get_prop(foo_device, foo_prop, ... )) {
> >     hw_error("your dtb is bad because ... !\n", ... );
> > }
> 
> What happens if the fdt is corrupt or malicious?  I guess we'll access
> memory beyond the end of blob.
> 
> This seems to be libfdt's fault.  I didn't see an API to validate the
> blob's size.
> 
> I'm "happy" with this patch but if fdt's can ever come from untrusted
> sources then we're in trouble.

Jon/David, can you confirm that libfdt has no way of check the size of
the fdt blob?

For example, if I pass a corrupt or malicious blob to libfdt, is there a
way to detect that or will it access memory beyond the end of the blob
as we query the device tree?

Stefan

Re: [Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep

[David Gibson]

On Wed, Aug 15, 2012 at 02:41:56PM +0100, Stefan Hajnoczi wrote:
> On Sat, Aug 11, 2012 at 01:33:42PM +0100, Stefan Hajnoczi wrote:
> > On Sat, Aug 11, 2012 at 12:11 AM, Peter Crosthwaite
> > <peter.crosthwaite@petalogix.com> wrote:
> > > On Fri, Aug 10, 2012 at 11:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> > >> On Fri, Aug 10, 2012 at 01:54:26PM +1000, Peter A. G. Crosthwaite wrote:
> > >>> The sizep arg is populated with the size of the loaded device tree. Since this
> > >>> is one of those informational "please populate" type arguments it should be
> > >>> optional. Guarded writes to *sizep against NULL accordingly.
> > >>>
> > >>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
> > >>> Acked-by: Alexander Graf <agraf@suse.de>
> > >>> ---
> > >>>  device_tree.c |    8 ++++++--
> > >>>  1 files changed, 6 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/device_tree.c b/device_tree.c
> > >>> index d7a9b6b..641a48a 100644
> > >>> --- a/device_tree.c
> > >>> +++ b/device_tree.c
> > >>> @@ -71,7 +71,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
> > >>>      int ret;
> > >>>      void *fdt = NULL;
> > >>>
> > >>> -    *sizep = 0;
> > >>> +    if (sizep) {
> > >>> +        *sizep = 0;
> > >>> +    }
> > >>>      dt_size = get_image_size(filename_path);
> > >>>      if (dt_size < 0) {
> > >>>          printf("Unable to get size of device tree file '%s'\n",
> > >>> @@ -104,7 +106,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
> > >>>              filename_path);
> > >>>          goto fail;
> > >>>      }
> > >>> -    *sizep = dt_size;
> > >>> +    if (sizep) {
> > >>> +        *sizep = dt_size;
> > >>> +    }
> > >>
> > >> What can the caller do with this void* buffer without knowing its size?
> > >>
> > >
> > > Sanity check the machine:
> > >
> > > dtb = load_device_tree( ... ); //dont care how big it is
> > > foo = fdt_gep_prop( dtb, ... );
> > > if (foo != object_get_prop(foo_device, foo_prop, ... )) {
> > >     hw_error("your dtb is bad because ... !\n", ... );
> > > }
> > 
> > What happens if the fdt is corrupt or malicious?  I guess we'll access
> > memory beyond the end of blob.
> > 
> > This seems to be libfdt's fault.  I didn't see an API to validate the
> > blob's size.
> > 
> > I'm "happy" with this patch but if fdt's can ever come from untrusted
> > sources then we're in trouble.
> 
> Jon/David, can you confirm that libfdt has no way of check the size of
> the fdt blob?

That's not rentirely true.

> For example, if I pass a corrupt or malicious blob to libfdt, is there a
> way to detect that or will it access memory beyond the end of the blob
> as we query the device tree?

So, libfdt does trust the blob size that's given in the blob header,
since libfdt itself doesn't really have any other source for the
blob/buffer size.  If you have another source for your buffer size
though, you can check that quite easily against fdt_totalsize(blob)
(which returns the header value).  If you can think of a helper
function that would make this easier, I'd be happy to add it to
libfdt.

Once the header size is validated, though, libfdt *is* supposed to be
safe against a corrupt or malicious blob.  I can't guarantee that we
don't have bugs here, but any crash on malicious data I do consider a
bug and will fix once I'm aware of it.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep

[Stefan Hajnoczi]

On Thu, Aug 16, 2012 at 3:14 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Wed, Aug 15, 2012 at 02:41:56PM +0100, Stefan Hajnoczi wrote:
>> On Sat, Aug 11, 2012 at 01:33:42PM +0100, Stefan Hajnoczi wrote:
>> > On Sat, Aug 11, 2012 at 12:11 AM, Peter Crosthwaite
>> > <peter.crosthwaite@petalogix.com> wrote:
>> > > On Fri, Aug 10, 2012 at 11:42 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> > >> On Fri, Aug 10, 2012 at 01:54:26PM +1000, Peter A. G. Crosthwaite wrote:
>> > >>> The sizep arg is populated with the size of the loaded device tree. Since this
>> > >>> is one of those informational "please populate" type arguments it should be
>> > >>> optional. Guarded writes to *sizep against NULL accordingly.
>> > >>>
>> > >>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> > >>> Acked-by: Alexander Graf <agraf@suse.de>
>> > >>> ---
>> > >>>  device_tree.c |    8 ++++++--
>> > >>>  1 files changed, 6 insertions(+), 2 deletions(-)
>> > >>>
>> > >>> diff --git a/device_tree.c b/device_tree.c
>> > >>> index d7a9b6b..641a48a 100644
>> > >>> --- a/device_tree.c
>> > >>> +++ b/device_tree.c
>> > >>> @@ -71,7 +71,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>> > >>>      int ret;
>> > >>>      void *fdt = NULL;
>> > >>>
>> > >>> -    *sizep = 0;
>> > >>> +    if (sizep) {
>> > >>> +        *sizep = 0;
>> > >>> +    }
>> > >>>      dt_size = get_image_size(filename_path);
>> > >>>      if (dt_size < 0) {
>> > >>>          printf("Unable to get size of device tree file '%s'\n",
>> > >>> @@ -104,7 +106,9 @@ void *load_device_tree(const char *filename_path, int *sizep)
>> > >>>              filename_path);
>> > >>>          goto fail;
>> > >>>      }
>> > >>> -    *sizep = dt_size;
>> > >>> +    if (sizep) {
>> > >>> +        *sizep = dt_size;
>> > >>> +    }
>> > >>
>> > >> What can the caller do with this void* buffer without knowing its size?
>> > >>
>> > >
>> > > Sanity check the machine:
>> > >
>> > > dtb = load_device_tree( ... ); //dont care how big it is
>> > > foo = fdt_gep_prop( dtb, ... );
>> > > if (foo != object_get_prop(foo_device, foo_prop, ... )) {
>> > >     hw_error("your dtb is bad because ... !\n", ... );
>> > > }
>> >
>> > What happens if the fdt is corrupt or malicious?  I guess we'll access
>> > memory beyond the end of blob.
>> >
>> > This seems to be libfdt's fault.  I didn't see an API to validate the
>> > blob's size.
>> >
>> > I'm "happy" with this patch but if fdt's can ever come from untrusted
>> > sources then we're in trouble.
>>
>> Jon/David, can you confirm that libfdt has no way of check the size of
>> the fdt blob?
>
> That's not rentirely true.
>
>> For example, if I pass a corrupt or malicious blob to libfdt, is there a
>> way to detect that or will it access memory beyond the end of the blob
>> as we query the device tree?
>
> So, libfdt does trust the blob size that's given in the blob header,
> since libfdt itself doesn't really have any other source for the
> blob/buffer size.  If you have another source for your buffer size
> though, you can check that quite easily against fdt_totalsize(blob)
> (which returns the header value).  If you can think of a helper
> function that would make this easier, I'd be happy to add it to
> libfdt.
>
> Once the header size is validated, though, libfdt *is* supposed to be
> safe against a corrupt or malicious blob.  I can't guarantee that we
> don't have bugs here, but any crash on malicious data I do consider a
> bug and will fix once I'm aware of it.

David:

fdt_check_header() does not check off_dt_struct, off_dt_strings,
off_mem_rsvmap, size_dt_strings, size_dt_struct against the blob size.
 For example, fdt_get_mem_rsv() will access out-of-bounds memory if
off_mem_rsvmap is invalid.  Or another example, fdt_offset_ptr() does
bounds checking on offset + len but only against the size_dt_struct
header field, which was never checked against the blob size.

Having the user check fdt_totalsize(blob) is not enough.  libfdt
itself needs to use the blob's external size to validate the fdt
header.  Something like:

/**
 * fdt_check_header_size - sanity check a device tree's size
 * @fdt: pointer to a flattened device tree
 * @size: fdt size in bytes
 *
 * fdt_check_header_size() checks that the given flattened
 * device tree header describes a data layout that fits within
 * the given size limit.  Use this to check untrusted fdt input
 * immediately after calling fdt_check_header() and before calling
 * other functions.
 *
 * returns:
 *     0, if the fdt fits within the given size limit
 *     -FDT_ERR_NOSPACE, fdt would exceed given size
 *     -FDT_ERR_BADMAGIC,
 *     -FDT_ERR_BADVERSION, standard meanings
 */
int fdt_check_blob_size(const void *fdt, size_t size);

Also, fdt_string() documentation says the function returns NULL if
stroffset is out of bounds.  The implementation does not check and
will return an out-of-bounds pointer.

Peter:

When libfdt adds the fdt_check_block_size() function then the QEMU
patch is no longer useful since the header size should be validate
(this requires a non-NULL size argument).  I suggest we drop the
patch.

Stefan