[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH] device_tree: load_device_tree():
From: |
David Gibson |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH] device_tree: load_device_tree(): Allow NULL sizep |
Date: |
Thu, 16 Aug 2012 12:14:11 +1000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
> > <address@hidden> wrote:
> > > On Fri, Aug 10, 2012 at 11:42 PM, Stefan Hajnoczi <address@hidden> 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 <address@hidden>
> > >>> Acked-by: Alexander Graf <address@hidden>
> > >>> ---
> > >>> 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