[PATCH] discover/platform: platform boot args append after verification

[Samuel Mendoza-Jonas]

On Mon, 2018-06-11 at 08:24 +1000, Brett Grandbois wrote:
> In a signed-boot environment, the cmdline options are verified along
> with the kernel, which is to be expected.  However this can cause
> problems in environments where the system needs to make a runtime
> adjustment to the cmdline args as now the signature will fail.  There
> is currently boot hook support for adjusting args but that happens
> before verification and will therefore fail, and also having a callout
> to a possible modified script/app seems to be too much of a security
> hole.  After some experimentation it appears the best solution to this
> issue is to add a new platform callout that is invoked after
> verification as part of the kexec cmdline args append.  Since the
> platform(s) are compiled-in they can't be (easily) altered on the system
> and the default NULL behavior of the platform system means that it must
> be explicitly invoked by a platform designer, which would be an implicit
> acknowledgement that the possible security hole is acceptable.
> Runtime update of dtb's in a signed-boot environment will also face this
> issue but that will be addressed separately.

Hi Brett,

This is a pretty clean solution to the problem, however modifying boot
arguments at all has been something we've tried to stay away from in
general, let alone modifying signed arguments :)
I already have an idea of what the aim is here but would you mind giving
a bit of a description of your use case so we can think about it in
context?

Cheers,
Sam

> 
> Signed-off-by: Brett Grandbois <brett.grandbois at opengear.com>
> ---
>  discover/boot.c     | 24 ++++++++++++++++++++----
>  discover/platform.c |  7 +++++++
>  discover/platform.h |  4 ++++
>  3 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/discover/boot.c b/discover/boot.c
> index 2a0d333..c66264a 100644
> --- a/discover/boot.c
> +++ b/discover/boot.c
> @@ -62,6 +62,7 @@ static int kexec_load(struct boot_task *boot_task)
>  	struct process *process;
>  	char *s_initrd = NULL;
>  	char *s_args = NULL;
> +	char *p_args = NULL;
>  	const char *argv[7];
>  	char *s_dtb = NULL;
>  	const char **p;
> @@ -121,10 +122,19 @@ static int kexec_load(struct boot_task *boot_task)
>  		*p++ = s_dtb;		 /* 4 */
>  	}
>  
> -	s_args = talloc_asprintf(boot_task, "--append=%s",
> -				boot_task->args ?: "\"\"");
> -	assert(s_args);
> -	*p++ = s_args;		/* 5 */
> +	p_args = platform_boot_args_append(boot_task);
> +
> +	if (p_args || (boot_task->args && strlen(boot_task->args))) {
> +		if (p_args && (boot_task->args && strlen(boot_task->args)))
> +			s_args = talloc_asprintf(boot_task, "--append=%s %s",
> +						boot_task->args, p_args);
> +		else
> +			s_args = talloc_asprintf(boot_task, "--append=%s",
> +						p_args ?: boot_task->args);
> +
> +		assert(s_args);
> +		*p++ = s_args;		/* 5 */
> +	}
>  
>  	*p++ = local_image;		/* 6 */
>  	*p++ = NULL;			/* 7 */
> @@ -581,6 +591,12 @@ struct boot_task *boot(void *ctx, struct discover_boot_option *opt,
>  			talloc_free(boot_task);
>  			return NULL;
>  		}
> +		/*
> +		 * no args is valid but the expectation is that the sigfile
> +		 * will have hashed ""
> +		 */
> +		if (!boot_task->args)
> +			boot_task->args = talloc_strdup(boot_task, "");
>  	}
>  
>  	image_res = add_boot_resource(boot_task, _("kernel image"), image,
> diff --git a/discover/platform.c b/discover/platform.c
> index cc6306f..93cc4b7 100644
> --- a/discover/platform.c
> +++ b/discover/platform.c
> @@ -209,6 +209,13 @@ int platform_get_sysinfo(struct system_info *info)
>  	return -1;
>  }
>  
> +char *platform_boot_args_append(const struct boot_task *task)
> +{
> +	if (platform && platform->boot_args_append)
> +		return platform->boot_args_append(platform, task);
> +	return NULL;
> +}
> +
>  int config_set(struct config *newconfig)
>  {
>  	int rc;
> diff --git a/discover/platform.h b/discover/platform.h
> index 5aa8e3f..9960c7d 100644
> --- a/discover/platform.h
> +++ b/discover/platform.h
> @@ -2,6 +2,7 @@
>  #define PLATFORM_H
>  
>  #include <types/types.h>
> +#include "boot.h"
>  
>  struct platform {
>  	const char	*name;
> @@ -11,6 +12,8 @@ struct platform {
>  	void		(*pre_boot)(struct platform *,
>  				const struct config *);
>  	int		(*get_sysinfo)(struct platform *, struct system_info *);
> +	char*		(*boot_args_append)(struct platform *,
> +					    const struct boot_task*);
>  	uint16_t	dhcp_arch_id;
>  	void		*platform_data;
>  };
> @@ -20,6 +23,7 @@ int platform_fini(void);
>  const struct platform *platform_get(void);
>  int platform_get_sysinfo(struct system_info *info);
>  void platform_pre_boot(void);
> +char *platform_boot_args_append(const struct boot_task*);
>  
>  /* configuration interface */
>  const struct config *config_get(void);