Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32862
to review the following change.
Change subject: device_tree: Add version checks ......................................................................
device_tree: Add version checks
This patch adds a few more sanity checks to the FDT header parsing to make sure that our code can support the version that is passed in.
This patch was adapted from depthcharge's http://crosreview.com/1536384
Change-Id: I06c112f540213c8db7c2455c2e8a4e8e4f337b78 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/include/device_tree.h M src/lib/device_tree.c M src/lib/fit.c 3 files changed, 23 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/32862/1
diff --git a/src/include/device_tree.h b/src/include/device_tree.h index 14be832..96902b8 100644 --- a/src/include/device_tree.h +++ b/src/include/device_tree.h @@ -33,7 +33,7 @@ uint32_t reserve_map_offset;
uint32_t version; - uint32_t last_compatible_version; + uint32_t last_comp_version;
uint32_t boot_cpuid_phys;
@@ -42,6 +42,7 @@ };
#define FDT_HEADER_MAGIC 0xd00dfeed +#define FDT_SUPPORTED_VERSION 17 #define FDT_TOKEN_BEGIN_NODE 1 #define FDT_TOKEN_END_NODE 2 #define FDT_TOKEN_PROPERTY 3 diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index bb40eee..7a3128e 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -220,6 +220,24 @@ const struct fdt_header *header = (const struct fdt_header *)blob; tree->header = header;
+ uint32_t magic = be32toh(header->magic); + uint32_t version = be32toh(header->version); + uint32_t last_comp_version = be32toh(header->last_comp_version); + + if (magic != FDT_HEADER_MAGIC) { + printk(BIOS_DEBUG, "Invalid device tree magic %#.8x!\n", magic); + return NULL; + } + if (last_comp_version > FDT_SUPPORTED_VERSION) { + printk(BIOS_DEBUG, "Unsupported device tree version %u(>=%u)\n", + version, last_comp_version); + return NULL; + } + if (version > FDT_SUPPORTED_VERSION) + printk(BIOS_DEBUG, + "NOTE: FDT version %u too new, should add support!\n", + version); + uint32_t struct_offset = be32toh(header->structure_offset); uint32_t strings_offset = be32toh(header->strings_offset); uint32_t reserve_offset = be32toh(header->reserve_map_offset); diff --git a/src/lib/fit.c b/src/lib/fit.c index d15641d..c98ba2f 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -423,19 +423,17 @@
struct fit_config_node *fit_load(void *fit) { - struct fdt_header *header = (struct fdt_header *)fit; struct fit_image_node *image; struct fit_config_node *config; struct compat_string_entry *compat_node;
printk(BIOS_DEBUG, "FIT: Loading FIT from %p\n", fit);
- if (be32toh(header->magic) != FDT_HEADER_MAGIC) { - printk(BIOS_ERR, "FIT: Bad header magic value 0x%08x.\n", - be32toh(header->magic)); + struct device_tree *tree = fdt_unflatten(fit); + if (!tree) { + printk(BIOS_ERR, "ERROR: Failed to unflatten FIT image!\n"); return NULL; } - struct device_tree *tree = fdt_unflatten(fit);
const char *default_config_name = NULL; struct fit_config_node *default_config = NULL;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32862 )
Change subject: device_tree: Add version checks ......................................................................
Patch Set 4: Code-Review+1
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32862 )
Change subject: device_tree: Add version checks ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/32862/5/src/include/device_tree.h File src/include/device_tree.h:
https://review.coreboot.org/#/c/32862/5/src/include/device_tree.h@36 PS5, Line 36: last_comp_version isn't the old name better?
https://review.coreboot.org/#/c/32862/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32862/5/src/lib/device_tree.c@237 PS5, Line 237: printk(BIOS_DEBUG, : "NOTE: FDT version %u too new, should add support!\n", : version); so we're not failing or skip?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32862 )
Change subject: device_tree: Add version checks ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/#/c/32862/5/src/include/device_tree.h File src/include/device_tree.h:
https://review.coreboot.org/#/c/32862/5/src/include/device_tree.h@36 PS5, Line 36: last_comp_version
isn't the old name better?
This is how it's called in the official device tree specification (https://www.devicetree.org/downloads/devicetree-specification-v0.1-20160524.... page 39).
https://review.coreboot.org/#/c/32862/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32862/5/src/lib/device_tree.c@237 PS5, Line 237: printk(BIOS_DEBUG, : "NOTE: FDT version %u too new, should add support!\n", : version);
so we're not failing or skip?
This case means last_comp_version is <= our version but version is >, so the device tree is still compatible with us, it's just a hint to tell us that we might want to update our code (I think you asked me to add this line on the depthcharge review, didn't you?).
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32862 )
Change subject: device_tree: Add version checks ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32862 )
Change subject: device_tree: Add version checks ......................................................................
device_tree: Add version checks
This patch adds a few more sanity checks to the FDT header parsing to make sure that our code can support the version that is passed in.
This patch was adapted from depthcharge's http://crosreview.com/1536384
Change-Id: I06c112f540213c8db7c2455c2e8a4e8e4f337b78 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32862 Reviewed-by: Hung-Te Lin hungte@chromium.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/include/device_tree.h M src/lib/device_tree.c M src/lib/fit.c 3 files changed, 23 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Hung-Te Lin: Looks good to me, approved
diff --git a/src/include/device_tree.h b/src/include/device_tree.h index 14be832..96902b8 100644 --- a/src/include/device_tree.h +++ b/src/include/device_tree.h @@ -33,7 +33,7 @@ uint32_t reserve_map_offset;
uint32_t version; - uint32_t last_compatible_version; + uint32_t last_comp_version;
uint32_t boot_cpuid_phys;
@@ -42,6 +42,7 @@ };
#define FDT_HEADER_MAGIC 0xd00dfeed +#define FDT_SUPPORTED_VERSION 17 #define FDT_TOKEN_BEGIN_NODE 1 #define FDT_TOKEN_END_NODE 2 #define FDT_TOKEN_PROPERTY 3 diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index bb40eee..7a3128e 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -220,6 +220,24 @@ const struct fdt_header *header = (const struct fdt_header *)blob; tree->header = header;
+ uint32_t magic = be32toh(header->magic); + uint32_t version = be32toh(header->version); + uint32_t last_comp_version = be32toh(header->last_comp_version); + + if (magic != FDT_HEADER_MAGIC) { + printk(BIOS_DEBUG, "Invalid device tree magic %#.8x!\n", magic); + return NULL; + } + if (last_comp_version > FDT_SUPPORTED_VERSION) { + printk(BIOS_DEBUG, "Unsupported device tree version %u(>=%u)\n", + version, last_comp_version); + return NULL; + } + if (version > FDT_SUPPORTED_VERSION) + printk(BIOS_DEBUG, + "NOTE: FDT version %u too new, should add support!\n", + version); + uint32_t struct_offset = be32toh(header->structure_offset); uint32_t strings_offset = be32toh(header->strings_offset); uint32_t reserve_offset = be32toh(header->reserve_map_offset); diff --git a/src/lib/fit.c b/src/lib/fit.c index d15641d..c98ba2f 100644 --- a/src/lib/fit.c +++ b/src/lib/fit.c @@ -423,19 +423,17 @@
struct fit_config_node *fit_load(void *fit) { - struct fdt_header *header = (struct fdt_header *)fit; struct fit_image_node *image; struct fit_config_node *config; struct compat_string_entry *compat_node;
printk(BIOS_DEBUG, "FIT: Loading FIT from %p\n", fit);
- if (be32toh(header->magic) != FDT_HEADER_MAGIC) { - printk(BIOS_ERR, "FIT: Bad header magic value 0x%08x.\n", - be32toh(header->magic)); + struct device_tree *tree = fdt_unflatten(fit); + if (!tree) { + printk(BIOS_ERR, "ERROR: Failed to unflatten FIT image!\n"); return NULL; } - struct device_tree *tree = fdt_unflatten(fit);
const char *default_config_name = NULL; struct fit_config_node *default_config = NULL;
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)