Re: [PATCH v5 11/13] powerpc: Allow userspace to set device tree properties in kexec_file_load

[Samuel Mendoza-Jonas]

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@xxxxxxxxxxxxxx>
> Signed-off-by: Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxxxxxxx>
> ---
> Â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