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

[Stefan Hajnoczi <stefanha@gmail.com>]

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