discover/platform: platform boot args append after verification

Message ID 1528669446-28531-1-git-send-email-brett.grandbois@opengear.com
State New
Headers show
Series
  • discover/platform: platform boot args append after verification
Related show

Commit Message

Grandbois, Brett June 10, 2018, 10:24 p.m.
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.

Signed-off-by: Brett Grandbois <brett.grandbois@opengear.com>
---
 discover/boot.c     | 24 ++++++++++++++++++++----
 discover/platform.c |  7 +++++++
 discover/platform.h |  4 ++++
 3 files changed, 31 insertions(+), 4 deletions(-)

Comments

Samuel Mendoza-Jonas July 2, 2018, 1:38 a.m. | #1
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@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);
Grandbois, Brett July 3, 2018, 12:19 a.m. | #2
On 02/07/18 11:38, Samuel Mendoza-Jonas wrote:
> 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

Sure Sam,

So the basic idea is that this is a replacement mechanism for the boot 
hooks in a signed boot environment, specifically in this case for boot args.

In all situations, the boot hooks run right before kexec_load(), where 
they are able to modify the args, dtb, even the image or initrd.  While 
still can be run in a signed-boot environment, any modifications done by 
the hooks will almost certainly cause a signature failure as those are 
performed in kexec_load(), which again as noted above is run after the 
hooks.  This is a problem in environments where signed-boot is enabled 
but system modifications are still necessary.  As an example, for our 
platform we have signed-boot enabled and also a requirement for 
persistent storage and runtime configuration of the kernel console 
settings.  This is an x86/ACPI environment so we don't have dtb's, the 
only way to configure this at runtime is appending a 'console=XX' to the 
kernel boot args.  But since the cmdline args as specified in the conf 
files are signed, appending the console configuration will violate the 
signature and the boot will fail.  I believe the powerpc platform has 
something similar but achieves it via dtb, which would also fail in a 
signed-boot environment.  We have another boot arg flag we need to pass 
that has to do with our updater (RAUC slot if you're interested) but I 
think the console is a good simple example to highlight the issue.

So I did quite a bit of experimentation and determined the least of all 
the evils is to allow the system to append boot args after the signature 
check.  I admit it is opening up a security hole, but it is implemented 
as a platform callout.  By default undefined platform callouts are not 
run so nothing happens in the default case.  In situations like ours we 
have to explicitly define the platform callout to do this, and doing so 
have acknowledged that this functionality is a higher priority than the 
possible security hole we are opening.  Hmm, maybe a good future feature 
is a configure option to specify which platform source files are to be 
included?  If anyone is keen on that I'm happy to take it on.

A more locked-down approach would be to have an explicit list of args 
that could be set by the system and appended directly, like for example 
the global 'boot_console' if defined is always appended as a 'console=', 
but that lacks flexibility and I think would just cause continual 
requests for new args to be added.

> 
>>
>> Signed-off-by: Brett Grandbois <brett.grandbois@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);
>
Samuel Mendoza-Jonas July 3, 2018, 6:17 a.m. | #3
On Tue, 2018-07-03 at 10:19 +1000, Brett Grandbois wrote:
> 
> On 02/07/18 11:38, Samuel Mendoza-Jonas wrote:
> > 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
> 
> Sure Sam,
> 
> So the basic idea is that this is a replacement mechanism for the boot 
> hooks in a signed boot environment, specifically in this case for boot args.
> 
> In all situations, the boot hooks run right before kexec_load(), where 
> they are able to modify the args, dtb, even the image or initrd.  While 
> still can be run in a signed-boot environment, any modifications done by 
> the hooks will almost certainly cause a signature failure as those are 
> performed in kexec_load(), which again as noted above is run after the 
> hooks.  This is a problem in environments where signed-boot is enabled 
> but system modifications are still necessary.  As an example, for our 
> platform we have signed-boot enabled and also a requirement for 
> persistent storage and runtime configuration of the kernel console 
> settings.  This is an x86/ACPI environment so we don't have dtb's, the 
> only way to configure this at runtime is appending a 'console=XX' to the 
> kernel boot args.  But since the cmdline args as specified in the conf 
> files are signed, appending the console configuration will violate the 
> signature and the boot will fail.  I believe the powerpc platform has 
> something similar but achieves it via dtb, which would also fail in a 
> signed-boot environment.  We have another boot arg flag we need to pass 
> that has to do with our updater (RAUC slot if you're interested) but I 
> think the console is a good simple example to highlight the issue.

One idea that came up while discussing this was possibly allowing
multiple signature files; if you know that there's a finite set of
'console=$tty' or 'rauc_param=foo' parameters that are allowable, could
we have a group of signatures that match each permutation? Then we can
still "actually" validate the signature when booting.

The platform callout is probably the least scary way of doing this but it
means taking very close care in that callout to make sure Petitboot can't
be tricked into appending something unintended, otherwise we'll
"validate" something that could be malicious.

> 
> So I did quite a bit of experimentation and determined the least of all 
> the evils is to allow the system to append boot args after the signature 
> check.  I admit it is opening up a security hole, but it is implemented 
> as a platform callout.  By default undefined platform callouts are not 
> run so nothing happens in the default case.  In situations like ours we 
> have to explicitly define the platform callout to do this, and doing so 
> have acknowledged that this functionality is a higher priority than the 
> possible security hole we are opening.  Hmm, maybe a good future feature 
> is a configure option to specify which platform source files are to be 
> included?  If anyone is keen on that I'm happy to take it on.
> 
> A more locked-down approach would be to have an explicit list of args 
> that could be set by the system and appended directly, like for example 
> the global 'boot_console' if defined is always appended as a 'console=', 
> but that lacks flexibility and I think would just cause continual 
> requests for new args to be added.
> 
> > 
> > > 
> > > Signed-off-by: Brett Grandbois <brett.grandbois@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);
Grandbois, Brett July 3, 2018, 10:32 p.m. | #4
On 03/07/18 16:17, Samuel Mendoza-Jonas wrote:
> On Tue, 2018-07-03 at 10:19 +1000, Brett Grandbois wrote:
>>
>> On 02/07/18 11:38, Samuel Mendoza-Jonas wrote:
>>> 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
>>
>> Sure Sam,
>>
>> So the basic idea is that this is a replacement mechanism for the boot
>> hooks in a signed boot environment, specifically in this case for boot args.
>>
>> In all situations, the boot hooks run right before kexec_load(), where
>> they are able to modify the args, dtb, even the image or initrd.  While
>> still can be run in a signed-boot environment, any modifications done by
>> the hooks will almost certainly cause a signature failure as those are
>> performed in kexec_load(), which again as noted above is run after the
>> hooks.  This is a problem in environments where signed-boot is enabled
>> but system modifications are still necessary.  As an example, for our
>> platform we have signed-boot enabled and also a requirement for
>> persistent storage and runtime configuration of the kernel console
>> settings.  This is an x86/ACPI environment so we don't have dtb's, the
>> only way to configure this at runtime is appending a 'console=XX' to the
>> kernel boot args.  But since the cmdline args as specified in the conf
>> files are signed, appending the console configuration will violate the
>> signature and the boot will fail.  I believe the powerpc platform has
>> something similar but achieves it via dtb, which would also fail in a
>> signed-boot environment.  We have another boot arg flag we need to pass
>> that has to do with our updater (RAUC slot if you're interested) but I
>> think the console is a good simple example to highlight the issue.
> 
> One idea that came up while discussing this was possibly allowing
> multiple signature files; if you know that there's a finite set of
> 'console=$tty' or 'rauc_param=foo' parameters that are allowable, could
> we have a group of signatures that match each permutation? Then we can
> still "actually" validate the signature when booting.

That can get messy very quickly.  For the very simple case of 2 RAUC 
slot + 2 console options is 4 different cmdline signatures.  Start 
adding in data rate, partiy, etc, and the number of combinations start 
exploding.  Then there's the question of how the signature is actually 
selected.  In this case you could probably get away with a new boot hook 
parameter rather than a platform callout to do some sort of runtime 
cmdline sig file update.  For a very small amount of options that would 
probably be sufficient but limits future flexibility as new options are 
needed.  This is of course the hardest part of security: where do you 
draw the line between security and flexibility?  IMHO a compiled-in 
in-house developed platform callout is by definition an acceptance of 
the flexibility tradeoff over the hard signature security.  Would a 
configure enable option help alleviate concerns?  In that case the 
callout has to be defined and the call explicitly enabled at build time?

> 
> The platform callout is probably the least scary way of doing this but it
> means taking very close care in that callout to make sure Petitboot can't
> be tricked into appending something unintended, otherwise we'll
> "validate" something that could be malicious.
> 
>>
>> So I did quite a bit of experimentation and determined the least of all
>> the evils is to allow the system to append boot args after the signature
>> check.  I admit it is opening up a security hole, but it is implemented
>> as a platform callout.  By default undefined platform callouts are not
>> run so nothing happens in the default case.  In situations like ours we
>> have to explicitly define the platform callout to do this, and doing so
>> have acknowledged that this functionality is a higher priority than the
>> possible security hole we are opening.  Hmm, maybe a good future feature
>> is a configure option to specify which platform source files are to be
>> included?  If anyone is keen on that I'm happy to take it on.
>>
>> A more locked-down approach would be to have an explicit list of args
>> that could be set by the system and appended directly, like for example
>> the global 'boot_console' if defined is always appended as a 'console=',
>> but that lacks flexibility and I think would just cause continual
>> requests for new args to be added.
>>
>>>
>>>>
>>>> Signed-off-by: Brett Grandbois <brett.grandbois@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);
>
Samuel Mendoza-Jonas July 9, 2018, 1:19 a.m. | #5
On Wed, 2018-07-04 at 08:32 +1000, Brett Grandbois wrote:
> 
> On 03/07/18 16:17, Samuel Mendoza-Jonas wrote:
> > On Tue, 2018-07-03 at 10:19 +1000, Brett Grandbois wrote:
> > > 
> > > On 02/07/18 11:38, Samuel Mendoza-Jonas wrote:
> > > > 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
> > > 
> > > Sure Sam,
> > > 
> > > So the basic idea is that this is a replacement mechanism for the boot
> > > hooks in a signed boot environment, specifically in this case for boot args.
> > > 
> > > In all situations, the boot hooks run right before kexec_load(), where
> > > they are able to modify the args, dtb, even the image or initrd.  While
> > > still can be run in a signed-boot environment, any modifications done by
> > > the hooks will almost certainly cause a signature failure as those are
> > > performed in kexec_load(), which again as noted above is run after the
> > > hooks.  This is a problem in environments where signed-boot is enabled
> > > but system modifications are still necessary.  As an example, for our
> > > platform we have signed-boot enabled and also a requirement for
> > > persistent storage and runtime configuration of the kernel console
> > > settings.  This is an x86/ACPI environment so we don't have dtb's, the
> > > only way to configure this at runtime is appending a 'console=XX' to the
> > > kernel boot args.  But since the cmdline args as specified in the conf
> > > files are signed, appending the console configuration will violate the
> > > signature and the boot will fail.  I believe the powerpc platform has
> > > something similar but achieves it via dtb, which would also fail in a
> > > signed-boot environment.  We have another boot arg flag we need to pass
> > > that has to do with our updater (RAUC slot if you're interested) but I
> > > think the console is a good simple example to highlight the issue.
> > 
> > One idea that came up while discussing this was possibly allowing
> > multiple signature files; if you know that there's a finite set of
> > 'console=$tty' or 'rauc_param=foo' parameters that are allowable, could
> > we have a group of signatures that match each permutation? Then we can
> > still "actually" validate the signature when booting.
> 
> That can get messy very quickly.  For the very simple case of 2 RAUC 
> slot + 2 console options is 4 different cmdline signatures.  Start 
> adding in data rate, partiy, etc, and the number of combinations start 
> exploding.

I have been imagining the scenario slightly differently in that while all these
combinations exist, in a given environment only a subset would actually be
allowed. If you need that kind of flexibility combined with signed boot then
that is definitely going to get messy. I guess my question is, does it make
sense to have that kind of flexibility in the signed boot scenario?

> Then there's the question of how the signature is actually 
> selected.  In this case you could probably get away with a new boot hook 
> parameter rather than a platform callout to do some sort of runtime 
> cmdline sig file update.  For a very small amount of options that would 
> probably be sufficient but limits future flexibility as new options are 
> needed.  This is of course the hardest part of security: where do you 
> draw the line between security and flexibility?  IMHO a compiled-in 
> in-house developed platform callout is by definition an acceptance of 
> the flexibility tradeoff over the hard signature security.  Would a 
> configure enable option help alleviate concerns?  In that case the 
> callout has to be defined and the call explicitly enabled at build time?

I reckon the patch as it is currently implemented is fine - nothing will
happen unless a platform has defined that function - but your idea below
about having an option to selectively build platform files would definitely
add an extra layer of reassurance :)

Cheers,
Sam

> 
> > 
> > The platform callout is probably the least scary way of doing this but it
> > means taking very close care in that callout to make sure Petitboot can't
> > be tricked into appending something unintended, otherwise we'll
> > "validate" something that could be malicious.
> > 
> > > 
> > > So I did quite a bit of experimentation and determined the least of all
> > > the evils is to allow the system to append boot args after the signature
> > > check.  I admit it is opening up a security hole, but it is implemented
> > > as a platform callout.  By default undefined platform callouts are not
> > > run so nothing happens in the default case.  In situations like ours we
> > > have to explicitly define the platform callout to do this, and doing so
> > > have acknowledged that this functionality is a higher priority than the
> > > possible security hole we are opening.  Hmm, maybe a good future feature
> > > is a configure option to specify which platform source files are to be
> > > included?  If anyone is keen on that I'm happy to take it on.
> > > 
> > > A more locked-down approach would be to have an explicit list of args
> > > that could be set by the system and appended directly, like for example
> > > the global 'boot_console' if defined is always appended as a 'console=',
> > > but that lacks flexibility and I think would just cause continual
> > > requests for new args to be added.
> > > 
> > > > 
> > > > > 
> > > > > Signed-off-by: Brett Grandbois <brett.grandbois@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);
Samuel Mendoza-Jonas July 30, 2018, 1:52 a.m. | #6
On Mon, 2018-07-09 at 11:19 +1000, Samuel Mendoza-Jonas wrote:
> On Wed, 2018-07-04 at 08:32 +1000, Brett Grandbois wrote:
> > 
> > On 03/07/18 16:17, Samuel Mendoza-Jonas wrote:
> > > On Tue, 2018-07-03 at 10:19 +1000, Brett Grandbois wrote:
> > > > 
> > > > On 02/07/18 11:38, Samuel Mendoza-Jonas wrote:
> > > > > 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
> > > > 
> > > > Sure Sam,
> > > > 
> > > > So the basic idea is that this is a replacement mechanism for the boot
> > > > hooks in a signed boot environment, specifically in this case for boot args.
> > > > 
> > > > In all situations, the boot hooks run right before kexec_load(), where
> > > > they are able to modify the args, dtb, even the image or initrd.  While
> > > > still can be run in a signed-boot environment, any modifications done by
> > > > the hooks will almost certainly cause a signature failure as those are
> > > > performed in kexec_load(), which again as noted above is run after the
> > > > hooks.  This is a problem in environments where signed-boot is enabled
> > > > but system modifications are still necessary.  As an example, for our
> > > > platform we have signed-boot enabled and also a requirement for
> > > > persistent storage and runtime configuration of the kernel console
> > > > settings.  This is an x86/ACPI environment so we don't have dtb's, the
> > > > only way to configure this at runtime is appending a 'console=XX' to the
> > > > kernel boot args.  But since the cmdline args as specified in the conf
> > > > files are signed, appending the console configuration will violate the
> > > > signature and the boot will fail.  I believe the powerpc platform has
> > > > something similar but achieves it via dtb, which would also fail in a
> > > > signed-boot environment.  We have another boot arg flag we need to pass
> > > > that has to do with our updater (RAUC slot if you're interested) but I
> > > > think the console is a good simple example to highlight the issue.
> > > 
> > > One idea that came up while discussing this was possibly allowing
> > > multiple signature files; if you know that there's a finite set of
> > > 'console=$tty' or 'rauc_param=foo' parameters that are allowable, could
> > > we have a group of signatures that match each permutation? Then we can
> > > still "actually" validate the signature when booting.
> > 
> > That can get messy very quickly.  For the very simple case of 2 RAUC 
> > slot + 2 console options is 4 different cmdline signatures.  Start 
> > adding in data rate, partiy, etc, and the number of combinations start 
> > exploding.
> 
> I have been imagining the scenario slightly differently in that while all these
> combinations exist, in a given environment only a subset would actually be
> allowed. If you need that kind of flexibility combined with signed boot then
> that is definitely going to get messy. I guess my question is, does it make
> sense to have that kind of flexibility in the signed boot scenario?
> 
> > Then there's the question of how the signature is actually 
> > selected.  In this case you could probably get away with a new boot hook 
> > parameter rather than a platform callout to do some sort of runtime 
> > cmdline sig file update.  For a very small amount of options that would 
> > probably be sufficient but limits future flexibility as new options are 
> > needed.  This is of course the hardest part of security: where do you 
> > draw the line between security and flexibility?  IMHO a compiled-in 
> > in-house developed platform callout is by definition an acceptance of 
> > the flexibility tradeoff over the hard signature security.  Would a 
> > configure enable option help alleviate concerns?  In that case the 
> > callout has to be defined and the call explicitly enabled at build time?
> 
> I reckon the patch as it is currently implemented is fine - nothing will
> happen unless a platform has defined that function - but your idea below
> about having an option to selectively build platform files would definitely
> add an extra layer of reassurance :)

Lo and behold, Geoff & Ge Song's series happens to include exactly this:
http://patchwork.ozlabs.org/patch/948898/

> 
> Cheers,
> Sam
> 
> > 
> > > 
> > > The platform callout is probably the least scary way of doing this but it
> > > means taking very close care in that callout to make sure Petitboot can't
> > > be tricked into appending something unintended, otherwise we'll
> > > "validate" something that could be malicious.
> > > 
> > > > 
> > > > So I did quite a bit of experimentation and determined the least of all
> > > > the evils is to allow the system to append boot args after the signature
> > > > check.  I admit it is opening up a security hole, but it is implemented
> > > > as a platform callout.  By default undefined platform callouts are not
> > > > run so nothing happens in the default case.  In situations like ours we
> > > > have to explicitly define the platform callout to do this, and doing so
> > > > have acknowledged that this functionality is a higher priority than the
> > > > possible security hole we are opening.  Hmm, maybe a good future feature
> > > > is a configure option to specify which platform source files are to be
> > > > included?  If anyone is keen on that I'm happy to take it on.
> > > > 
> > > > A more locked-down approach would be to have an explicit list of args
> > > > that could be set by the system and appended directly, like for example
> > > > the global 'boot_console' if defined is always appended as a 'console=',
> > > > but that lacks flexibility and I think would just cause continual
> > > > requests for new args to be added.
> > > > 
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Brett Grandbois <brett.grandbois@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);
> 
> _______________________________________________
> Petitboot mailing list
> Petitboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/petitboot

Patch

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);