| Message ID | 1528669446-28531-1-git-send-email-brett.grandbois@opengear.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | discover/platform: platform boot args append after verification | expand |
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);
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); >
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);
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); >
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);
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
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);
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(-)