| Message ID | 1470956898-5991-12-git-send-email-bauerman@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 2016-08-11 at 20:08 -0300, Thiago Jung Bauermann wrote: > Implement the arch_kexec_verify_buffer hook to verify that a device > tree blob passed by userspace via kexec_file_load contains only nodes > and properties from a whitelist. > > In elf64_load we merge those properties into the device tree that > will be passed to the next kernel. > > Suggested-by: Michael Ellerman <mpe@ellerman.id.au> > Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> > --- > arch/powerpc/include/asm/kexec.h | 1 + > arch/powerpc/kernel/kexec_elf_64.c | 9 ++ > arch/powerpc/kernel/machine_kexec_64.c | 242 +++++++++++++++++++++++++++++++++ > 3 files changed, 252 insertions(+) > > diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h > index f263cc867891..31bc64e07c8f 100644 > --- a/arch/powerpc/include/asm/kexec.h > +++ b/arch/powerpc/include/asm/kexec.h > @@ -99,6 +99,7 @@ int setup_purgatory(struct kimage *image, const void *slave_code, > int setup_new_fdt(void *fdt, unsigned long initrd_load_addr, > unsigned long initrd_len, const char *cmdline); > bool find_debug_console(const void *fdt, int chosen_node); > +int merge_partial_dtb(void *to, const void *from); > #endif /* CONFIG_KEXEC_FILE */ > > #else /* !CONFIG_KEXEC */ > diff --git a/arch/powerpc/kernel/kexec_elf_64.c b/arch/powerpc/kernel/kexec_elf_64.c > index 49cba9509464..1b902ad66e2a 100644 > --- a/arch/powerpc/kernel/kexec_elf_64.c > +++ b/arch/powerpc/kernel/kexec_elf_64.c > @@ -210,6 +210,15 @@ void *elf64_load(struct kimage *image, char *kernel_buf, > goto out; > } > > + /* Add nodes and properties from the DTB passed by userspace. */ > + if (image->dtb_buf) { > + ret = merge_partial_dtb(fdt, image->dtb_buf); > + if (ret) { > + pr_err("Error merging partial device tree.\n"); > + goto out; > + } > + } > + > ret = setup_new_fdt(fdt, initrd_load_addr, initrd_len, cmdline); > if (ret) > goto out; > diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c > index 527f98efe651..a484a6346146 100644 > --- a/arch/powerpc/kernel/machine_kexec_64.c > +++ b/arch/powerpc/kernel/machine_kexec_64.c > @@ -35,6 +35,7 @@ > #include <asm/kexec_elf_64.h> > > #define SLAVE_CODE_SIZE 256 > +#define MAX_DT_PATH 512 > > #ifdef CONFIG_KEXEC_FILE > static struct kexec_file_ops *kexec_file_loaders[] = { > @@ -908,4 +909,245 @@ bool find_debug_console(const void *fdt, int chosen_node) > return false; > } > > +/** > + * struct allowed_node - a node in the whitelist and its allowed properties. > + * @name: node name or full node path > + * @properties: NULL-terminated array of names or name=value pairs > + * > + * If name starts with /, then the node has to be at the specified path in > + * the device tree (including unit addresses for all nodes in the path). > + * If it doesn't, then the node can be anywhere in the device tree. > + * > + * An entry in properties can specify a string value that the property must > + * have by using the "name=value" format. If the entry ends with =, it means > + * that the property must be empty. > + */ > +static struct allowed_node { > + const char *name; > + const char *properties[9]; > +} allowed_nodes[] = { > + { > + .name = "/chosen", > + .properties = { > + "stdout-path", > + "linux,stdout-path", > + NULL, > + } > + }, > + { > + .name = "vga", > + .properties = { > + "device_type=display", > + "assigned-addresses", > + "width", > + "height", > + "depth", > + "little-endian=", > + "linux,opened=", > + "linux,boot-display=",ss > + NULL, > + } > + }, > +}; Hi Thiago, As much as this solves problems for *me*, I suspect adding 'vga' here might be the subject of some discussion. Having /chosen whitelisted makes sense on it's own, but 'vga' and its properties are very specific without much explanation. If everyone's happy to have it there, cool! If not, I have the majority of a patch that handles the original reason for these property updates separately in the kernel rather than from userspace. If needed I'll clean it up and we can handle it that way. Cheers, Sam
Hello Sam, Thanks for the quick response. Am Freitag, 12 August 2016, 10:45:00 schrieb Samuel Mendoza-Jonas: > On Thu, 2016-08-11 at 20:08 -0300, Thiago Jung Bauermann wrote: > > @@ -908,4 +909,245 @@ bool find_debug_console(const void *fdt, int > > chosen_node) return false; > > } > > > > +/** > > + * struct allowed_node - a node in the whitelist and its allowed > > properties. + * @name: node name or full node path > > + * @properties: NULL-terminated array of names or > > name=value pairs + * > > + * If name starts with /, then the node has to be at the specified path > > in + * the device tree (including unit addresses for all nodes in the > > path). + * If it doesn't, then the node can be anywhere in the device > > tree. + * > > + * An entry in properties can specify a string value that the property > > must + * have by using the "name=value" format. If the entry ends with > > =, it means + * that the property must be empty. > > + */ > > +static struct allowed_node { > > + const char *name; > > + const char *properties[9]; > > +} allowed_nodes[] = { > > + { > > + .name = "/chosen", > > + .properties = { > > + "stdout-path", > > + "linux,stdout-path", > > + NULL, > > + } > > + }, > > + { > > + .name = "vga", > > + .properties = { > > + "device_type=display", > > + "assigned-addresses", > > + "width", > > + "height", > > + "depth", > > + "little-endian=", > > + "linux,opened=", > > + "linux,boot-display=",ss > > + NULL, > > + } > > + }, > > +}; > > Hi Thiago, > > As much as this solves problems for *me*, I suspect adding 'vga' here > might be the subject of some discussion. Having /chosen whitelisted makes > sense on it's own, but 'vga' and its properties are very specific without > much explanation. > > If everyone's happy to have it there, cool! If not, I have the majority > of a patch that handles the original reason for these property updates > separately in the kernel rather than from userspace. If needed I'll clean > it up and we can handle it that way. Ok, that's good to know. I'm fine with it either way. In any case, 'vga' in this patch also serves a good real-life example of a non-trivial binding outside of /chosen that we might want to whitelist in the future.
diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h index f263cc867891..31bc64e07c8f 100644 --- a/arch/powerpc/include/asm/kexec.h +++ b/arch/powerpc/include/asm/kexec.h @@ -99,6 +99,7 @@ int setup_purgatory(struct kimage *image, const void *slave_code, int setup_new_fdt(void *fdt, unsigned long initrd_load_addr, unsigned long initrd_len, const char *cmdline); bool find_debug_console(const void *fdt, int chosen_node); +int merge_partial_dtb(void *to, const void *from); #endif /* CONFIG_KEXEC_FILE */ #else /* !CONFIG_KEXEC */ diff --git a/arch/powerpc/kernel/kexec_elf_64.c b/arch/powerpc/kernel/kexec_elf_64.c index 49cba9509464..1b902ad66e2a 100644 --- a/arch/powerpc/kernel/kexec_elf_64.c +++ b/arch/powerpc/kernel/kexec_elf_64.c @@ -210,6 +210,15 @@ void *elf64_load(struct kimage *image, char *kernel_buf, goto out; } + /* Add nodes and properties from the DTB passed by userspace. */ + if (image->dtb_buf) { + ret = merge_partial_dtb(fdt, image->dtb_buf); + if (ret) { + pr_err("Error merging partial device tree.\n"); + goto out; + } + } + ret = setup_new_fdt(fdt, initrd_load_addr, initrd_len, cmdline); if (ret) goto out; diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c index 527f98efe651..a484a6346146 100644 --- a/arch/powerpc/kernel/machine_kexec_64.c +++ b/arch/powerpc/kernel/machine_kexec_64.c @@ -35,6 +35,7 @@ #include <asm/kexec_elf_64.h> #define SLAVE_CODE_SIZE 256 +#define MAX_DT_PATH 512 #ifdef CONFIG_KEXEC_FILE static struct kexec_file_ops *kexec_file_loaders[] = { @@ -908,4 +909,245 @@ bool find_debug_console(const void *fdt, int chosen_node) return false; } +/** + * struct allowed_node - a node in the whitelist and its allowed properties. + * @name: node name or full node path + * @properties: NULL-terminated array of names or name=value pairs + * + * If name starts with /, then the node has to be at the specified path in + * the device tree (including unit addresses for all nodes in the path). + * If it doesn't, then the node can be anywhere in the device tree. + * + * An entry in properties can specify a string value that the property must + * have by using the "name=value" format. If the entry ends with =, it means + * that the property must be empty. + */ +static struct allowed_node { + const char *name; + const char *properties[9]; +} allowed_nodes[] = { + { + .name = "/chosen", + .properties = { + "stdout-path", + "linux,stdout-path", + NULL, + } + }, + { + .name = "vga", + .properties = { + "device_type=display", + "assigned-addresses", + "width", + "height", + "depth", + "little-endian=", + "linux,opened=", + "linux,boot-display=", + NULL, + } + }, +}; + +/** + * verify_properties() - verify that all properties in a node are allowed + * @properties: Array of allowed properties in the node. + * @fdt: Device tree blob. + * @node: Offset to node being verified. + * + * Return: 0 on success, negative errno on error. + */ +static int verify_properties(const char *properties[], const void *fdt, int node) +{ + int prop; + + for (prop = fdt_first_property_offset(fdt, node); prop >= 0; + prop = fdt_next_property_offset(fdt, prop)) { + const char *prop_name; + const void *prop_val; + int i; + + prop_val = fdt_getprop_by_offset(fdt, prop, &prop_name, NULL); + if (prop_val == NULL) { + pr_debug("Error reading device tree.\n"); + return -EINVAL; + } + + for (i = 0; properties[i] != NULL; i++) { + size_t len; + const char *allowed_prop = properties[i]; + + len = strchrnul(allowed_prop, '=') - allowed_prop; + if (!strncmp(allowed_prop, prop_name, len)) { + if (strchr(allowed_prop, '=') != NULL) + /* We only support checking strings. */ + if (strcmp(allowed_prop + len + 1, prop_val)) { + pr_debug("Device tree property %s has an invalid value for node %s.\n", + prop_name, fdt_get_name(fdt, node, NULL)); + return -EINVAL; + } + + break; + } + } + + if (properties[i] == NULL) { + pr_debug("Device tree property not allowed for node %s: %s\n", + fdt_get_name(fdt, node, NULL), prop_name); + return -EINVAL; + } + } + + return 0; +} + +int arch_kexec_verify_buffer(enum kexec_file_type type, const void *buf, + unsigned long size) +{ + int node; + + if (type != KEXEC_FILE_TYPE_PARTIAL_DTB) { + pr_debug("Invalid file type.\n"); + return -EINVAL; + } + + if (fdt_check_header(buf)) { + pr_debug("Malformed device tree.\n"); + return -EINVAL; + } + + if (fdt_num_mem_rsv(buf) != 0) { + pr_debug("Device tree has memory reservations.\n"); + return -EINVAL; + } + + /* + * Check that the device tree only has nodes and properties listed + * in the whitelist. + */ + for (node = fdt_next_node(buf, -1, NULL); node >= 0; + node = fdt_next_node(buf, node, NULL)) { + int i; + + for (i = 0; i < ARRAY_SIZE(allowed_nodes); i++) { + int ret; + + if (allowed_nodes[i].name[0] == '/') { + char path[MAX_DT_PATH]; + + if (fdt_get_path(buf, node, path, sizeof(path))) { + pr_debug("Error reading device tree.\n"); + return -EINVAL; + } + + if (!strcmp(allowed_nodes[i].name, path)) { + ret = verify_properties(allowed_nodes[i].properties, + buf, node); + if (ret) + return ret; + + break; + } + } else { + const char *name; + size_t len; + + name = fdt_get_name(buf, node, NULL); + if (name == NULL) { + pr_debug("Error reading device tree.\n"); + return -EINVAL; + } + + len = strchrnul(name, '@') - name; + if (!strncmp(allowed_nodes[i].name, name, len)) { + ret = verify_properties(allowed_nodes[i].properties, + buf, node); + if (ret) + return ret; + + break; + } + } + } + + /* + * If a node isn't in the whitelist but has at least one subnode + * and no properties we allow it, since there may be a + * whitelisted node under it. + */ + if (i == ARRAY_SIZE(allowed_nodes) && + (fdt_first_property_offset(buf, node) != -FDT_ERR_NOTFOUND || + fdt_first_subnode(buf, node) == -FDT_ERR_NOTFOUND)) { + pr_debug("Device tree node not allowed: %s\n", + fdt_get_name(buf, node, NULL)); + return -EINVAL; + } + } + + return 0; +} + +/** + * merge_partial_dtb() - copy all nodes and properties from one DTB to another + * + * Return: 0 on success, negative errno on error. + */ +int merge_partial_dtb(void *to, const void *from) +{ + int from_node; + + for (from_node = fdt_next_node(from, -1, NULL); from_node >= 0; + from_node = fdt_next_node(from, from_node, NULL)) { + int prop, to_node; + char path[MAX_DT_PATH]; + + if (fdt_get_path(from, from_node, path, sizeof(path))) { + pr_debug("Error reading device tree.\n"); + return -EINVAL; + } + + to_node = fdt_path_offset(to, path); + if (to_node == -FDT_ERR_NOTFOUND) { + /* We allow creating /chosen if it doesn't exist. */ + if (strcmp(path, "/chosen")) { + pr_debug("%s doesn't exist in the device tree.\n", + path); + return -EINVAL; + } + + to_node = fdt_add_subnode(to, fdt_path_offset(to, "/"), + "chosen"); + if (to_node < 0) { + pr_debug("Error creating the /chosen node.\n"); + return -EINVAL; + } + } else if (to_node < 0) { + pr_debug("Error reading device tree.\n"); + return -EINVAL; + } + + for (prop = fdt_first_property_offset(from, from_node); prop >= 0; + prop = fdt_next_property_offset(from, prop)) { + const char *name; + const void *val; + int len, ret; + + val = fdt_getprop_by_offset(from, prop, &name, &len); + if (val == NULL) { + pr_debug("Error reading device tree.\n"); + return -EINVAL; + } + + ret = fdt_setprop(to, to_node, name, val, len); + if (ret) { + pr_debug("Error writing new device tree.\n"); + return -EINVAL; + } + } + } + + return 0; +} + #endif /* CONFIG_KEXEC_FILE */
Implement the arch_kexec_verify_buffer hook to verify that a device tree blob passed by userspace via kexec_file_load contains only nodes and properties from a whitelist. In elf64_load we merge those properties into the device tree that will be passed to the next kernel. Suggested-by: Michael Ellerman <mpe@ellerman.id.au> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com> --- arch/powerpc/include/asm/kexec.h | 1 + arch/powerpc/kernel/kexec_elf_64.c | 9 ++ arch/powerpc/kernel/machine_kexec_64.c | 242 +++++++++++++++++++++++++++++++++ 3 files changed, 252 insertions(+)