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.crosthwa...@petalogix.com> wrote: > > > On Fri, Aug 10, 2012 at 11:42 PM, Stefan Hajnoczi <stefa...@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.crosthwa...@petalogix.com> > > >>> Acked-by: Alexander Graf <ag...@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