| Message ID | 1344570866-28595-2-git-send-email-peter.crosthwaite@petalogix.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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.
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
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: