[PATCH] discover/platform: platform boot args append after verification
Brett Grandbois
brett.grandbois at opengear.com
Tue Jul 3 10:19:42 AEST 2018
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 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);
>
More information about the Petitboot
mailing list