| Message ID | 20190322073555.20889-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | device_tree: check device tree blob file size | expand |
On Fri, 22 Mar 2019 at 07:38, P J P <ppandit@redhat.com> wrote: > > From: Prasad J Pandit <pjp@fedoraproject.org> > > Device tree blob(dtb) file can not be larger than 2MB in size.[*] > Add check to avoid loading large dtb files in load_device_tree(), > and potential integer(dt_size) overflow. > > [*] linux.git/tree/Documentation/arm64/booting.txt This document is specific to aarch64, but the part of QEMU's device tree code being modified here is architecture independent. Cc'ing David Gibson who will probably know if there is an architecture-independent limit on DTB size we should be enforcing, or whether we are better just to have a check that avoids the overflow. It's also worth noting in the commit message that this is not a security problem -- even if the "add 10000 and double" calculation overflows, the load_image_size() function will not load more data into the buffer than will fit, so the behaviour will be to truncate the DTB. > Reported-by: Kurtis Miller <kurtis.miller@nccgroup.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > device_tree.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/device_tree.c b/device_tree.c > index 296278e12a..9059ee5545 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -79,9 +79,9 @@ void *load_device_tree(const char *filename_path, int *sizep) > > *sizep = 0; > dt_size = get_image_size(filename_path); > - if (dt_size < 0) { > - error_report("Unable to get size of device tree file '%s'", > - filename_path); > + if (dt_size < 0 || dt_size > FDT_MAX_SIZE) { > + error_report("Invalid size of device tree file: %s: %d", > + filename_path, dt_size); > goto fail; > } thanks -- PMM
+-- On Fri, 22 Mar 2019, Peter Maydell wrote --+ | This document is specific to aarch64, but the part of | QEMU's device tree code being modified here is | architecture independent. | | Cc'ing David Gibson who will probably know if there is | an architecture-independent limit on DTB size we should | be enforcing, or whether we are better just to have a check | that avoids the overflow. Thank you for CC'ing David. It seems Agraf did not receive email @suse.de. Current limit defined by FDT_MAX_SIZE is ~1MB. device_tree.c: #define FDT_MAX_SIZE 0x100000 | It's also worth noting in the commit message that this is | not a security problem -- even if the "add 10000 and double" | calculation overflows, the load_image_size() function will | not load more data into the buffer than will fit, so the | behaviour will be to truncate the DTB. True, load_image_size() helps to avoid buffer overflow issue. Proposed check (dt_size > FDT_MAX_SIZE) in this patch is to enforce same size limit as used in create_device_tree() and avoid loading large files and the said integer overflow. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On Fri, 22 Mar 2019 at 10:11, P J P <ppandit@redhat.com> wrote: > > +-- On Fri, 22 Mar 2019, Peter Maydell wrote --+ > | This document is specific to aarch64, but the part of > | QEMU's device tree code being modified here is > | architecture independent. > | > | Cc'ing David Gibson who will probably know if there is > | an architecture-independent limit on DTB size we should > | be enforcing, or whether we are better just to have a check > | that avoids the overflow. > > Thank you for CC'ing David. It seems Agraf did not receive email @suse.de. Yes, Alex's email has changed (I've updated the cc list). > Current limit defined by FDT_MAX_SIZE is ~1MB. But currently this is only used when creating a DT from scratch. > Proposed check (dt_size > FDT_MAX_SIZE) in this patch is to enforce same size > limit as used in create_device_tree() and avoid loading large files and the > said integer overflow. My worry is that this might possibly break existing working use cases which load a device tree that is larger than 1MB. Unless there's a cross-architecture justification for the 1MB limit it seems quite a low one to be enforcing (especially since the one limit we've found so far for aarch64 is 2MB, not 1MB). thanks -- PMM
On Fri, Mar 22, 2019 at 09:14:53AM +0000, Peter Maydell wrote: > On Fri, 22 Mar 2019 at 07:38, P J P <ppandit@redhat.com> wrote: > > > > From: Prasad J Pandit <pjp@fedoraproject.org> > > > > Device tree blob(dtb) file can not be larger than 2MB in size.[*] > > Add check to avoid loading large dtb files in load_device_tree(), > > and potential integer(dt_size) overflow. > > > > [*] linux.git/tree/Documentation/arm64/booting.txt > > This document is specific to aarch64, but the part of > QEMU's device tree code being modified here is > architecture independent. > > Cc'ing David Gibson who will probably know if there is > an architecture-independent limit on DTB size we should > be enforcing, or whether we are better just to have a check > that avoids the overflow. The only inherent limit to dtb size should be 2^31-1 bytes (the format uses signed 32-bit ints as offsets). Indeed there shouldn't be any architecture (as in instruction set) dependent limits either. There may however be more specific platform dependent limits. > It's also worth noting in the commit message that this is > not a security problem -- even if the "add 10000 and double" > calculation overflows, the load_image_size() function will > not load more data into the buffer than will fit, so the > behaviour will be to truncate the DTB. Yeah, you should probably make that hard error rather than truncating. If a system works with a truncated tree, it can only be by sheer accident.
On Fri, Mar 22, 2019 at 10:30:51AM +0000, Peter Maydell wrote: > On Fri, 22 Mar 2019 at 10:11, P J P <ppandit@redhat.com> wrote: > > > > +-- On Fri, 22 Mar 2019, Peter Maydell wrote --+ > > | This document is specific to aarch64, but the part of > > | QEMU's device tree code being modified here is > > | architecture independent. > > | > > | Cc'ing David Gibson who will probably know if there is > > | an architecture-independent limit on DTB size we should > > | be enforcing, or whether we are better just to have a check > > | that avoids the overflow. > > > > Thank you for CC'ing David. It seems Agraf did not receive email @suse.de. > > Yes, Alex's email has changed (I've updated the cc list). > > > Current limit defined by FDT_MAX_SIZE is ~1MB. > > But currently this is only used when creating a DT from scratch. Right, and AFAIK the only reason we have a fixed buffer size for that is because it avoids having to mess around with reallocation if we hit an -FDT_ERR_NOSPACE during creation.
Hello David,
+-- On Mon, 25 Mar 2019, David Gibson wrote --+
| The only inherent limit to dtb size should be 2^31-1 bytes (the format
| uses signed 32-bit ints as offsets).
~2GB of dtb?! Seems quite big to specify the h/w that a kernel is
going to run/boot on.
| Indeed there shouldn't be any architecture (as in instruction set)
| dependent limits either. There may however be more specific platform
| dependent limits.
$ find . -name \*.dts -exec ls -shXS --color {} \; | sort -grk1 | less -r
-> https://paste.fedoraproject.org/paste/~9p-lVWwX7jmngHMQjCBsg
Going through the .dts files in the Linux kernel tree, 64KB appears to top
the list of file sizes.
IMO, generic 2MB of dtb size limit is reasonable; Considering 64KB is the max
we are seeing, plus QEMU has FDT_MAX_SIZE defined to be 0x100000(~1MB), and
noone has complained that it's too small.
| Yeah, you should probably make that hard error rather than truncating.
| If a system works with a truncated tree, it can only be by sheer
| accident.
Yes, current patch would 'goto fail; if (dt_size > FDT_MAX_SIZE)'.
Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On Mon, 25 Mar 2019 at 10:34, P J P <ppandit@redhat.com> wrote: > IMO, generic 2MB of dtb size limit is reasonable; Considering 64KB is the max > we are seeing, plus QEMU has FDT_MAX_SIZE defined to be 0x100000(~1MB), and > noone has complained that it's too small. Noone has complained that it's too small because right now *we do not check against it* for the common case of "just load an external dtb". We should not be imposing an arbitrary limit within QEMU if we don't need to. Here, we do not need to. thanks -- PMM
+-- On Mon, 25 Mar 2019, Peter Maydell wrote --+ | Noone has complained that it's too small because right now *we do not check | against it* for the common case of "just load an external dtb". | | We should not be imposing an arbitrary limit within QEMU if we don't need | to. Here, we do not need to. Hmmn, it could help to avoid loading of truncated dtb files as David pointed out. Thank you. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
On Mon, 25 Mar 2019 at 16:11, P J P <ppandit@redhat.com> wrote: > > +-- On Mon, 25 Mar 2019, Peter Maydell wrote --+ > | Noone has complained that it's too small because right now *we do not check > | against it* for the common case of "just load an external dtb". > | > | We should not be imposing an arbitrary limit within QEMU if we don't need > | to. Here, we do not need to. > > Hmmn, it could help to avoid loading of truncated dtb files as David pointed > out. Yes, we don't want to do that. We can do that in a less arbitrary manner than "impose a flat 1MB limit". thanks -- PMM
On Mon, Mar 25, 2019 at 04:04:33PM +0530, P J P wrote: > Hello David, > > +-- On Mon, 25 Mar 2019, David Gibson wrote --+ > | The only inherent limit to dtb size should be 2^31-1 bytes (the format > | uses signed 32-bit ints as offsets). > > ~2GB of dtb?! Seems quite big to specify the h/w that a kernel is > going to run/boot on. Well, sure, but why would we impose a limit when we didn't have a need to.
diff --git a/device_tree.c b/device_tree.c index 296278e12a..9059ee5545 100644 --- a/device_tree.c +++ b/device_tree.c @@ -79,9 +79,9 @@ void *load_device_tree(const char *filename_path, int *sizep) *sizep = 0; dt_size = get_image_size(filename_path); - if (dt_size < 0) { - error_report("Unable to get size of device tree file '%s'", - filename_path); + if (dt_size < 0 || dt_size > FDT_MAX_SIZE) { + error_report("Invalid size of device tree file: %s: %d", + filename_path, dt_size); goto fail; }