diff mbox

[U-Boot,2/2] usbarmory: Add board_run_command() function

Message ID 20160620152149.21249-2-andrej@inversepath.com
State Accepted
Commit a02ab5eaff966ecf746bc4e90696c84efb4b113b
Delegated to: Stefano Babic
Headers show

Commit Message

andrej@inversepath.com June 20, 2016, 3:21 p.m. UTC 
From: Andrej Rosano <andrej@inversepath.com>

Define a default board_run_command() function. This function contains
the commands needed to boot the board when CLI is disabled (CONFIG_CMDLINE=n).

Signed-off-by: Andrej Rosano <andrej@inversepath.com>
---
 board/inversepath/usbarmory/usbarmory.c | 31 +++++++++++++++++++++++++++++++
 include/configs/usbarmory.h             | 11 +++++++----
 2 files changed, 38 insertions(+), 4 deletions(-)

Comments

Stefano Babic June 20, 2016, 3:46 p.m. UTC | #1 
Hallo Andrej,

On 20/06/2016 17:21, andrej@inversepath.com wrote:
> From: Andrej Rosano <andrej@inversepath.com>
> 
> Define a default board_run_command() function. This function contains
> the commands needed to boot the board when CLI is disabled (CONFIG_CMDLINE=n).
> 
> Signed-off-by: Andrej Rosano <andrej@inversepath.com>
> ---
>  board/inversepath/usbarmory/usbarmory.c | 31 +++++++++++++++++++++++++++++++
>  include/configs/usbarmory.h             | 11 +++++++----
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/board/inversepath/usbarmory/usbarmory.c b/board/inversepath/usbarmory/usbarmory.c
> index a809039..a11e3b3 100644
> --- a/board/inversepath/usbarmory/usbarmory.c
> +++ b/board/inversepath/usbarmory/usbarmory.c
> @@ -415,3 +415,34 @@ int checkboard(void)
>  	puts("Board: Inverse Path USB armory MkI\n");
>  	return 0;
>  }
> +
> +#ifndef CONFIG_CMDLINE
> +static char *ext2_argv[] = {
> +	"ext2load",
> +	"mmc",
> +	"0:1",
> +	USBARMORY_FIT_ADDR,
> +	USBARMORY_FIT_PATH
> +};
> +
> +static char *bootm_argv[] = {
> +	"bootm",
> +	USBARMORY_FIT_ADDR
> +};
> +
> +int board_run_command(const char *cmdline)
> +{
> +	printf("%s %s %s %s %s\n", ext2_argv[0], ext2_argv[1], ext2_argv[2],
> +	       ext2_argv[3], ext2_argv[4]);
> +
> +	if (do_ext2load(NULL, 0, 5, ext2_argv) != 0) {
> +		udelay(5*1000*1000);
> +		return 1;
> +	}
> +
> +	printf("%s %s\n", bootm_argv[0], bootm_argv[1]);
> +	do_bootm(NULL, 0, 2, bootm_argv);
> +
> +	return 1;
> +}

I ten to NACK this. You can do exactly the same with a U-Boot script,
and if you want to have this as default, you can change your default
environment. This is just a wrapper around the hush shell.


> +#endif
> diff --git a/include/configs/usbarmory.h b/include/configs/usbarmory.h
> index c0e093f..5484204 100644
> --- a/include/configs/usbarmory.h
> +++ b/include/configs/usbarmory.h
> @@ -17,16 +17,13 @@
>  #define CONFIG_SYS_FSL_CLK
>  #define CONFIG_BOARD_EARLY_INIT_F
>  #define CONFIG_MXC_GPIO
> +#define CONFIG_SYS_NO_FLASH
>  
>  #include <asm/arch/imx-regs.h>
>  
>  #include <config_distro_defaults.h>
>  
> -/* U-Boot commands */
> -
>  /* U-Boot environment */
> -#define CONFIG_ENV_OVERWRITE
> -#define CONFIG_SYS_NO_FLASH
>  #define CONFIG_ENV_OFFSET	(6 * 64 * 1024)
>  #define CONFIG_ENV_SIZE		(8 * 1024)
>  #define CONFIG_ENV_IS_IN_MMC
> @@ -101,6 +98,12 @@
>  	"console=ttymxc0,115200\0"				\
>  	BOOTENV
>  
> +#ifndef CONFIG_CMDLINE
> +#define CONFIG_BOOTARGS "console=ttymxc0,115200 root=/dev/mmcblk0p1 rootwait rw"
> +#define USBARMORY_FIT_PATH	"/boot/usbarmory.itb"
> +#define USBARMORY_FIT_ADDR	"0x70800000"
> +#endif
> +
>  /* Physical Memory Map */
>  #define CONFIG_NR_DRAM_BANKS		1
>  #define PHYS_SDRAM			CSD0_BASE_ADDR
> 

Best regards,
Stefano Babic
andrej@inversepath.com June 20, 2016, 4:18 p.m. UTC | #2 
Hi Stefano,

On Mon, Jun 20, 2016 at 05:46:52PM +0200, Stefano Babic wrote:
> Hallo Andrej,
> 
> On 20/06/2016 17:21, andrej@inversepath.com wrote:
> > From: Andrej Rosano <andrej@inversepath.com>
> > 
> > Define a default board_run_command() function. This function contains
> > the commands needed to boot the board when CLI is disabled (CONFIG_CMDLINE=n).
> > 
> > Signed-off-by: Andrej Rosano <andrej@inversepath.com>
> > ---
> >  board/inversepath/usbarmory/usbarmory.c | 31 +++++++++++++++++++++++++++++++
> >  include/configs/usbarmory.h             | 11 +++++++----
> >  2 files changed, 38 insertions(+), 4 deletions(-)
> > 
> > diff --git a/board/inversepath/usbarmory/usbarmory.c b/board/inversepath/usbarmory/usbarmory.c
> > index a809039..a11e3b3 100644
> > --- a/board/inversepath/usbarmory/usbarmory.c
> > +++ b/board/inversepath/usbarmory/usbarmory.c
> > @@ -415,3 +415,34 @@ int checkboard(void)
> >  	puts("Board: Inverse Path USB armory MkI\n");
> >  	return 0;
> >  }
> > +
> > +#ifndef CONFIG_CMDLINE
> > +static char *ext2_argv[] = {
> > +	"ext2load",
> > +	"mmc",
> > +	"0:1",
> > +	USBARMORY_FIT_ADDR,
> > +	USBARMORY_FIT_PATH
> > +};
> > +
> > +static char *bootm_argv[] = {
> > +	"bootm",
> > +	USBARMORY_FIT_ADDR
> > +};
> > +
> > +int board_run_command(const char *cmdline)
> > +{
> > +	printf("%s %s %s %s %s\n", ext2_argv[0], ext2_argv[1], ext2_argv[2],
> > +	       ext2_argv[3], ext2_argv[4]);
> > +
> > +	if (do_ext2load(NULL, 0, 5, ext2_argv) != 0) {
> > +		udelay(5*1000*1000);
> > +		return 1;
> > +	}
> > +
> > +	printf("%s %s\n", bootm_argv[0], bootm_argv[1]);
> > +	do_bootm(NULL, 0, 2, bootm_argv);
> > +
> > +	return 1;
> > +}
> 
> I ten to NACK this. You can do exactly the same with a U-Boot script,
> and if you want to have this as default, you can change your default
> environment. This is just a wrapper around the hush shell.

The intention of the patch is to boot the kernel while having the CLI disabled
(CONFIG_CMDLINE=n). The U-Boot script needs the CLI to be enabled AFAIK.

It is better having the CLI disabled when using the Verified Boot, otherwise
there are chances to bypass the FIT image verification (e.g. using md/mw
commands in case are available):

https://github.com/inversepath/usbarmory/blob/master/software/secure_boot/u-boot-2016.05_patches/0003-Disable-CLI.patch

Andrej

> 
> 
> > +#endif
> > diff --git a/include/configs/usbarmory.h b/include/configs/usbarmory.h
> > index c0e093f..5484204 100644
> > --- a/include/configs/usbarmory.h
> > +++ b/include/configs/usbarmory.h
> > @@ -17,16 +17,13 @@
> >  #define CONFIG_SYS_FSL_CLK
> >  #define CONFIG_BOARD_EARLY_INIT_F
> >  #define CONFIG_MXC_GPIO
> > +#define CONFIG_SYS_NO_FLASH
> >  
> >  #include <asm/arch/imx-regs.h>
> >  
> >  #include <config_distro_defaults.h>
> >  
> > -/* U-Boot commands */
> > -
> >  /* U-Boot environment */
> > -#define CONFIG_ENV_OVERWRITE
> > -#define CONFIG_SYS_NO_FLASH
> >  #define CONFIG_ENV_OFFSET	(6 * 64 * 1024)
> >  #define CONFIG_ENV_SIZE		(8 * 1024)
> >  #define CONFIG_ENV_IS_IN_MMC
> > @@ -101,6 +98,12 @@
> >  	"console=ttymxc0,115200\0"				\
> >  	BOOTENV
> >  
> > +#ifndef CONFIG_CMDLINE
> > +#define CONFIG_BOOTARGS "console=ttymxc0,115200 root=/dev/mmcblk0p1 rootwait rw"
> > +#define USBARMORY_FIT_PATH	"/boot/usbarmory.itb"
> > +#define USBARMORY_FIT_ADDR	"0x70800000"
> > +#endif
> > +
> >  /* Physical Memory Map */
> >  #define CONFIG_NR_DRAM_BANKS		1
> >  #define PHYS_SDRAM			CSD0_BASE_ADDR
> > 
> 
> Best regards,
> Stefano Babic
> 
> -- 
> =====================================================================
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
> =====================================================================

--
Andrej Rosano                                           Inverse Path Srl
<andrej@inversepath.com>                      http://www.inversepath.com

0x01939B21            5BB8 574E 68E8 D841 E18F  D5E9 CEAD E0CF 0193 9B21
Stefano Babic June 27, 2016, 10:38 a.m. UTC | #3 
Hi Andrej,

On 20/06/2016 18:18, Andrej Rosano wrote:

>>
>> I ten to NACK this. You can do exactly the same with a U-Boot script,
>> and if you want to have this as default, you can change your default
>> environment. This is just a wrapper around the hush shell.
> 
> The intention of the patch is to boot the kernel while having the CLI disabled
> (CONFIG_CMDLINE=n). The U-Boot script needs the CLI to be enabled AFAIK.
> 
> It is better having the CLI disabled when using the Verified Boot, otherwise
> there are chances to bypass the FIT image verification (e.g. using md/mw
> commands in case are available):

Why is it not enough to disable the CONSOLE ? I mean, if there is no
user interface (and this is done in a lot of ways, for example setting
stdin / stdout), there is no ways to bypass it because the interface is
not availabel. Or is there some other security issues I am not aware of ?

Best regards,
Stefano Babic
Simon Glass June 28, 2016, 6:43 p.m. UTC | #4 
Hi,

On 27 June 2016 at 03:38, Stefano Babic <sbabic@denx.de> wrote:
> Hi Andrej,
>
> On 20/06/2016 18:18, Andrej Rosano wrote:
>
>>>
>>> I ten to NACK this. You can do exactly the same with a U-Boot script,
>>> and if you want to have this as default, you can change your default
>>> environment. This is just a wrapper around the hush shell.
>>
>> The intention of the patch is to boot the kernel while having the CLI disabled
>> (CONFIG_CMDLINE=n). The U-Boot script needs the CLI to be enabled AFAIK.
>>
>> It is better having the CLI disabled when using the Verified Boot, otherwise
>> there are chances to bypass the FIT image verification (e.g. using md/mw
>> commands in case are available):
>
> Why is it not enough to disable the CONSOLE ? I mean, if there is no
> user interface (and this is done in a lot of ways, for example setting
> stdin / stdout), there is no ways to bypass it because the interface is
> not availabel. Or is there some other security issues I am not aware of ?

It is an extra level of security - providing a very simple command
execution instead of the general CLI. That is actually the original
purpose of board_run_command(). E.g. for Chrome OS we had an option to
either run the normal CLI or a simple (secure) one. Also see
cli_process_fdt() which provides for a 'bootsecure' mode, controlled
from the FDT.

Regards,
Simon
Stefano Babic June 29, 2016, 9:20 a.m. UTC | #5 
Hi Simon,

On 28/06/2016 20:43, Simon Glass wrote:
> Hi,
> 
> On 27 June 2016 at 03:38, Stefano Babic <sbabic@denx.de> wrote:
>> Hi Andrej,
>>
>> On 20/06/2016 18:18, Andrej Rosano wrote:
>>
>>>>
>>>> I ten to NACK this. You can do exactly the same with a U-Boot script,
>>>> and if you want to have this as default, you can change your default
>>>> environment. This is just a wrapper around the hush shell.
>>>
>>> The intention of the patch is to boot the kernel while having the CLI disabled
>>> (CONFIG_CMDLINE=n). The U-Boot script needs the CLI to be enabled AFAIK.
>>>
>>> It is better having the CLI disabled when using the Verified Boot, otherwise
>>> there are chances to bypass the FIT image verification (e.g. using md/mw
>>> commands in case are available):
>>
>> Why is it not enough to disable the CONSOLE ? I mean, if there is no
>> user interface (and this is done in a lot of ways, for example setting
>> stdin / stdout), there is no ways to bypass it because the interface is
>> not availabel. Or is there some other security issues I am not aware of ?
> 
> It is an extra level of security - providing a very simple command
> execution instead of the general CLI. That is actually the original
> purpose of board_run_command(). E.g. for Chrome OS we had an option to
> either run the normal CLI or a simple (secure) one. Also see
> cli_process_fdt() which provides for a 'bootsecure' mode, controlled
> from the FDT.

I see, thanks for explanation. My fear is that the process diverges and
boards start to embed U-Boot scripts inside the code, letting them not
very maintainable. But I have understood the issue and I put this patch
for merging in my queue.

Best regards,
Stefano
diff mbox

Patch

diff --git a/board/inversepath/usbarmory/usbarmory.c b/board/inversepath/usbarmory/usbarmory.c
index a809039..a11e3b3 100644
--- a/board/inversepath/usbarmory/usbarmory.c
+++ b/board/inversepath/usbarmory/usbarmory.c
@@ -415,3 +415,34 @@  int checkboard(void)
 	puts("Board: Inverse Path USB armory MkI\n");
 	return 0;
 }
+
+#ifndef CONFIG_CMDLINE
+static char *ext2_argv[] = {
+	"ext2load",
+	"mmc",
+	"0:1",
+	USBARMORY_FIT_ADDR,
+	USBARMORY_FIT_PATH
+};
+
+static char *bootm_argv[] = {
+	"bootm",
+	USBARMORY_FIT_ADDR
+};
+
+int board_run_command(const char *cmdline)
+{
+	printf("%s %s %s %s %s\n", ext2_argv[0], ext2_argv[1], ext2_argv[2],
+	       ext2_argv[3], ext2_argv[4]);
+
+	if (do_ext2load(NULL, 0, 5, ext2_argv) != 0) {
+		udelay(5*1000*1000);
+		return 1;
+	}
+
+	printf("%s %s\n", bootm_argv[0], bootm_argv[1]);
+	do_bootm(NULL, 0, 2, bootm_argv);
+
+	return 1;
+}
+#endif
diff --git a/include/configs/usbarmory.h b/include/configs/usbarmory.h
index c0e093f..5484204 100644
--- a/include/configs/usbarmory.h
+++ b/include/configs/usbarmory.h
@@ -17,16 +17,13 @@ 
 #define CONFIG_SYS_FSL_CLK
 #define CONFIG_BOARD_EARLY_INIT_F
 #define CONFIG_MXC_GPIO
+#define CONFIG_SYS_NO_FLASH
 
 #include <asm/arch/imx-regs.h>
 
 #include <config_distro_defaults.h>
 
-/* U-Boot commands */
-
 /* U-Boot environment */
-#define CONFIG_ENV_OVERWRITE
-#define CONFIG_SYS_NO_FLASH
 #define CONFIG_ENV_OFFSET	(6 * 64 * 1024)
 #define CONFIG_ENV_SIZE		(8 * 1024)
 #define CONFIG_ENV_IS_IN_MMC
@@ -101,6 +98,12 @@ 
 	"console=ttymxc0,115200\0"				\
 	BOOTENV
 
+#ifndef CONFIG_CMDLINE
+#define CONFIG_BOOTARGS "console=ttymxc0,115200 root=/dev/mmcblk0p1 rootwait rw"
+#define USBARMORY_FIT_PATH	"/boot/usbarmory.itb"
+#define USBARMORY_FIT_ADDR	"0x70800000"
+#endif
+
 /* Physical Memory Map */
 #define CONFIG_NR_DRAM_BANKS		1
 #define PHYS_SDRAM			CSD0_BASE_ADDR