On 08:50 Mon 13 Mar , Sascha Hauer wrote:
> On Thu, Mar 09, 2017 at 03:34:09PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > request confirmation before booting an unsigned image
> >
> > with a default timeout
> >
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx>
> > ---
> > commands/go.c | 7 +++++++
> > common/Kconfig | 8 ++++++++
> > common/Makefile | 1 +
> > common/bootm.c | 7 +++++++
> > common/secure_boot.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> > include/bootm.h | 1 +
> > include/secure_boot.h | 25 +++++++++++++++++++++++++
> > 7 files changed, 92 insertions(+)
> > create mode 100644 common/secure_boot.c
> > create mode 100644 include/secure_boot.h
> >
> > diff --git a/commands/go.c b/commands/go.c
> > index fb319b320..61c9ce43f 100644
> > --- a/commands/go.c
> > +++ b/commands/go.c
> > @@ -26,6 +26,7 @@
> > #include <fcntl.h>
> > #include <linux/ctype.h>
> > #include <errno.h>
> > +#include <secure_boot.h>
> >
> > static int do_go(int argc, char *argv[])
> > {
> > @@ -37,6 +38,12 @@ static int do_go(int argc, char *argv[])
> > if (argc < 2)
> > return COMMAND_ERROR_USAGE;
> >
> > + rcode = secure_boot_start_unsigned();
> > + if (rcode)
> > + return rcode ? 1 : 0;
>
> When rcode is true, then if rcode is true, return 1, otherwise return 0?
I forget we could return a negative error on command too
>
> > +
> > + rcode = 1;
> > +
> > if (!isdigit(*argv[1])) {
> > fd = open(argv[1], O_RDONLY);
> > if (fd < 0) {
> > diff --git a/common/Kconfig b/common/Kconfig
> > index f7ff04664..9e7d027a2 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -21,6 +21,9 @@ config HAS_KALLSYMS
> > config HAS_MODULES
> > bool
> >
> > +config HAS_SECURE_BOOT
> > + bool
> > +
> > config HAS_CACHE
> > bool
> > help
> > @@ -184,6 +187,11 @@ config NVVAR
> > while global variables can be changed during runtime without changing the
> > default.
> >
> > +config SECURE_BOOT_TIMEOUT
> > + prompt "Secure Boot unsigned boot timeout"
> > + int
> > + default 3
> > +
> > menu "memory layout"
> >
> > source "pbl/Kconfig"
> > diff --git a/common/Makefile b/common/Makefile
> > index 5f58c81d2..e57cc2c15 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_UBIFORMAT) += ubiformat.o
> > obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
> > obj-$(CONFIG_CONSOLE_RATP) += ratp.o
> > obj-$(CONFIG_BOOT) += boot.o
> > +obj-$(CONFIG_HAS_SECURE_BOOT) += secure_boot.o
> >
> > quiet_cmd_pwd_h = PWDH $@
> > ifdef CONFIG_PASSWORD
> > diff --git a/common/bootm.c b/common/bootm.c
> > index 81625d915..0ebf65681 100644
> > --- a/common/bootm.c
> > +++ b/common/bootm.c
> > @@ -23,6 +23,7 @@
> > #include <environment.h>
> > #include <linux/stat.h>
> > #include <magicvar.h>
> > +#include <secure_boot.h>
> >
> > static LIST_HEAD(handler_list);
> >
> > @@ -625,6 +626,12 @@ int bootm_boot(struct bootm_data *bootm_data)
> > printf("Passing control to %s handler\n", handler->name);
> > }
> >
> > + if (!handler->is_secure_supported && is_secure_mode()) {
> > + ret = secure_boot_start_unsigned();
> > + if (ret)
> > + goto err_out;
> > + }
> > +
> > ret = handler->bootm(data);
> > if (data->dryrun)
> > printf("Dryrun. Aborted\n");
> > diff --git a/common/secure_boot.c b/common/secure_boot.c
> > new file mode 100644
> > index 000000000..e625ef4e1
> > --- /dev/null
> > +++ b/common/secure_boot.c
> > @@ -0,0 +1,43 @@
> > +/*
> > + * Copyright (c) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx>
> > + *
> > + * Under GPLv2 Only
> > + */
> > +#include <common.h>
> > +#include <secure_boot.h>
> > +#include <console_countdown.h>
> > +#include <globalvar.h>
> > +#include <magicvar.h>
> > +#include <init.h>
> > +
> > +static unsigned int sb_confirm_timeout = CONFIG_SECURE_BOOT_TIMEOUT;
>
> Use globalvar_add_simple_int instead of putting this into the config.
>
> > +
> > +int secure_boot_start_unsigned(void)
>
> This function name is very misleading. It sounds like it starts an
> unsigned image, but this function doesn't start anything. Second guess
> is that it returns a boolean whether it's allowed to start an unsigned
> image, but actually it returns the opposite.
secure_boot_can_start_unsigned could be better
it return 0 if ok
an error otherwise
as we can return an security violation or other error
Best Regards,
J.
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
