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

[Stefan Hajnoczi <stefanha@gmail.com>]

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