[RFC 0/2] ARC: rework U-boot arguments handling

[RFC 0/2] ARC: rework U-boot arguments handling

[Eugeniy Paltsev]

Eugeniy Paltsev (2):
  ARC: U-boot: check arguments paranoidly
  ARC: U-boot: check magic number passed from u-boot

 arch/arc/kernel/head.S  |  7 ++--
 arch/arc/kernel/setup.c | 91 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 74 insertions(+), 24 deletions(-)

-- 
2.14.5

[RFC 0/2] ARC: rework U-boot arguments handling

[Eugeniy Paltsev]

Eugeniy Paltsev (2):
  ARC: U-boot: check arguments paranoidly
  ARC: U-boot: check magic number passed from u-boot

 arch/arc/kernel/head.S  |  7 ++--
 arch/arc/kernel/setup.c | 91 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 74 insertions(+), 24 deletions(-)

-- 
2.14.5

[RFC 1/2] ARC: U-boot: check arguments paranoidly

[Eugeniy Paltsev]

Handle U-boot arguments paranoidly:
 * don't allow to pass unknown tag.
 * try to use external device tree blob only if corresponding tag
   (TAG_DTB) is set.
 * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.

While I'm at it refactor U-boot arguments handling code.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 arch/arc/kernel/head.S  |  2 +-
 arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index 8b90d25a15cc..7095055bb874 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -95,7 +95,7 @@ ENTRY(stext)
 	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
 	;    r1 = magic number (board identity, unused as of now
 	;    r2 = pointer to uboot provided cmdline or external DTB in mem
-	; These are handled later in setup_arch()
+	; These are handled later in handle_uboot_args()
 	st	r0, [@uboot_tag]
 	st	r2, [@uboot_arg]
 #endif
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index feb90093e6b1..7edb35c26322 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -462,43 +462,64 @@ void setup_processor(void)
 	arc_chk_core_config();
 }
 
-static inline int is_kernel(unsigned long addr)
+static inline bool is_kernel(unsigned long addr)
 {
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
-		return 1;
-	return 0;
+	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
 }
 
-void __init setup_arch(char **cmdline_p)
+/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
+#define UBOOT_REV0P_TAG_NONE		0
+#define UBOOT_REV0P_TAG_CMDLINE		1
+#define UBOOT_REV0P_TAG_DTB		2
+
+void __init handle_uboot_args(void)
 {
+	bool append_boot_cmdline = false;
+	bool use_embedded_dtb = true;
+
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
+	/* check that we know this tag */
+	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
+	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
+	    uboot_tag != UBOOT_REV0P_TAG_DTB)
+		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
+
 	/* make sure that uboot passed pointer to cmdline/dtb is valid */
-	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
+	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
 		panic("Invalid uboot arg\n");
 
 	/* See if u-boot passed an external Device Tree blob */
-	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
-	if (!machine_desc)
+	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
+		machine_desc = setup_machine_fdt(uboot_arg);
+
+		/* external Device Tree blob is invalid - use embedded one */
+		use_embedded_dtb = !machine_desc;
+	}
+
+	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
+		append_boot_cmdline = true;
 #endif
-	{
-		/* No, so try the embedded one */
+
+	if (use_embedded_dtb) {
 		machine_desc = setup_machine_fdt(__dtb_start);
 		if (!machine_desc)
 			panic("Embedded DT invalid\n");
+	}
 
-		/*
-		 * If we are here, it is established that @uboot_arg didn't
-		 * point to DT blob. Instead if u-boot says it is cmdline,
-		 * append to embedded DT cmdline.
-		 * setup_machine_fdt() would have populated @boot_command_line
-		 */
-		if (uboot_tag == 1) {
-			/* Ensure a whitespace between the 2 cmdlines */
-			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
-			strlcat(boot_command_line, uboot_arg,
-				COMMAND_LINE_SIZE);
-		}
+	/*
+	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
+	 * to embedded DT cmdline.
+	 */
+	if (append_boot_cmdline) {
+		/* Ensure a whitespace between the 2 cmdlines */
+		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
 	}
+}
+
+void __init setup_arch(char **cmdline_p)
+{
+	handle_uboot_args();
 
 	/* Save unparsed command line copy for /proc/cmdline */
 	*cmdline_p = boot_command_line;
-- 
2.14.5

[RFC 1/2] ARC: U-boot: check arguments paranoidly

[Eugeniy Paltsev]

Handle U-boot arguments paranoidly:
 * don't allow to pass unknown tag.
 * try to use external device tree blob only if corresponding tag
   (TAG_DTB) is set.
 * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.

While I'm at it refactor U-boot arguments handling code.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
---
 arch/arc/kernel/head.S  |  2 +-
 arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index 8b90d25a15cc..7095055bb874 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -95,7 +95,7 @@ ENTRY(stext)
 	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
 	;    r1 = magic number (board identity, unused as of now
 	;    r2 = pointer to uboot provided cmdline or external DTB in mem
-	; These are handled later in setup_arch()
+	; These are handled later in handle_uboot_args()
 	st	r0, [@uboot_tag]
 	st	r2, [@uboot_arg]
 #endif
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index feb90093e6b1..7edb35c26322 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -462,43 +462,64 @@ void setup_processor(void)
 	arc_chk_core_config();
 }
 
-static inline int is_kernel(unsigned long addr)
+static inline bool is_kernel(unsigned long addr)
 {
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
-		return 1;
-	return 0;
+	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
 }
 
-void __init setup_arch(char **cmdline_p)
+/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
+#define UBOOT_REV0P_TAG_NONE		0
+#define UBOOT_REV0P_TAG_CMDLINE		1
+#define UBOOT_REV0P_TAG_DTB		2
+
+void __init handle_uboot_args(void)
 {
+	bool append_boot_cmdline = false;
+	bool use_embedded_dtb = true;
+
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
+	/* check that we know this tag */
+	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
+	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
+	    uboot_tag != UBOOT_REV0P_TAG_DTB)
+		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
+
 	/* make sure that uboot passed pointer to cmdline/dtb is valid */
-	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
+	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
 		panic("Invalid uboot arg\n");
 
 	/* See if u-boot passed an external Device Tree blob */
-	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
-	if (!machine_desc)
+	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
+		machine_desc = setup_machine_fdt(uboot_arg);
+
+		/* external Device Tree blob is invalid - use embedded one */
+		use_embedded_dtb = !machine_desc;
+	}
+
+	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
+		append_boot_cmdline = true;
 #endif
-	{
-		/* No, so try the embedded one */
+
+	if (use_embedded_dtb) {
 		machine_desc = setup_machine_fdt(__dtb_start);
 		if (!machine_desc)
 			panic("Embedded DT invalid\n");
+	}
 
-		/*
-		 * If we are here, it is established that @uboot_arg didn't
-		 * point to DT blob. Instead if u-boot says it is cmdline,
-		 * append to embedded DT cmdline.
-		 * setup_machine_fdt() would have populated @boot_command_line
-		 */
-		if (uboot_tag == 1) {
-			/* Ensure a whitespace between the 2 cmdlines */
-			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
-			strlcat(boot_command_line, uboot_arg,
-				COMMAND_LINE_SIZE);
-		}
+	/*
+	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
+	 * to embedded DT cmdline.
+	 */
+	if (append_boot_cmdline) {
+		/* Ensure a whitespace between the 2 cmdlines */
+		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
 	}
+}
+
+void __init setup_arch(char **cmdline_p)
+{
+	handle_uboot_args();
 
 	/* Save unparsed command line copy for /proc/cmdline */
 	*cmdline_p = boot_command_line;
-- 
2.14.5

[RFC 2/2] ARC: U-boot: check magic number passed from u-boot

[Eugeniy Paltsev]

In case of devboards we really often disable bootloader and load
Linux image in memory via JTAG. In case of using kernel with
CONFIG_ARC_UBOOT_SUPPORT enabled we may crash as we will try to
interpret some junk in a registers as a pointers to bootargs/etc
which aren't set by anyone in case of JTAG using.

Try to make it much less possible by check magic number and
'U-boot - kernel' ABI revision number passed from U-boot.
Ignore U-boot arguments if we got wrong magic number or unknown
ABI revision.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 arch/arc/kernel/head.S  |  5 ++++-
 arch/arc/kernel/setup.c | 26 ++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index 7095055bb874..3fb88ec62bc7 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -92,10 +92,13 @@ ENTRY(stext)
 
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
 	; Uboot - kernel ABI
+	;    r1 = bits [31:8] magic number, bits [7:0] uboot-kernel ABI revision
+	; ABI revision 0:
 	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
-	;    r1 = magic number (board identity, unused as of now
 	;    r2 = pointer to uboot provided cmdline or external DTB in mem
 	; These are handled later in handle_uboot_args()
+	st      r1, [@uboot_rev_magic]
+	mov	r1, 0			; errase magic from the register
 	st	r0, [@uboot_tag]
 	st	r2, [@uboot_arg]
 #endif
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 7edb35c26322..868dda3d4b43 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -36,6 +36,7 @@ unsigned int intr_to_DE_cnt;
 
 /* Part of U-boot ABI: see head.S */
 int __initdata uboot_tag;
+int __initdata uboot_rev_magic;
 char __initdata *uboot_arg;
 
 const struct machine_desc *machine_desc;
@@ -467,6 +468,10 @@ static inline bool is_kernel(unsigned long addr)
 	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
 }
 
+#define UBOOT_MAGIC_VALUE		0x567890
+#define UBOOT_MAGIC_GET(x)		(((x) & GENMASK(31, 8)) >> 8)
+#define UBOOT_REVISION_GET(x)		((x) & GENMASK(7, 0))
+
 /* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
 #define UBOOT_REV0P_TAG_NONE		0
 #define UBOOT_REV0P_TAG_CMDLINE		1
@@ -478,6 +483,25 @@ void __init handle_uboot_args(void)
 	bool use_embedded_dtb = true;
 
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
+	/* check that we got correct magic */
+	if (UBOOT_MAGIC_GET(uboot_rev_magic) != UBOOT_MAGIC_VALUE) {
+		pr_warn("Invalid magic '%06lx' is passed from uboot, uboot args ingnored\n",
+			UBOOT_MAGIC_GET(uboot_rev_magic));
+
+		goto ignore_uboot_args;
+	}
+
+	/*
+	 * check that we know this U-boot args ABI revision.
+	 * as for today we only have one revision - '0'.
+	 */
+	if (UBOOT_REVISION_GET(uboot_rev_magic) != 0) {
+		pr_warn("Unknown args revision '%02lx' is passed from uboot, uboot args ingnored\n",
+			UBOOT_REVISION_GET(uboot_rev_magic));
+
+		goto ignore_uboot_args;
+	}
+
 	/* check that we know this tag */
 	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
 	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
@@ -498,6 +522,8 @@ void __init handle_uboot_args(void)
 
 	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
 		append_boot_cmdline = true;
+
+ignore_uboot_args:
 #endif
 
 	if (use_embedded_dtb) {
-- 
2.14.5

[RFC 2/2] ARC: U-boot: check magic number passed from u-boot

[Eugeniy Paltsev]

In case of devboards we really often disable bootloader and load
Linux image in memory via JTAG. In case of using kernel with
CONFIG_ARC_UBOOT_SUPPORT enabled we may crash as we will try to
interpret some junk in a registers as a pointers to bootargs/etc
which aren't set by anyone in case of JTAG using.

Try to make it much less possible by check magic number and
'U-boot - kernel' ABI revision number passed from U-boot.
Ignore U-boot arguments if we got wrong magic number or unknown
ABI revision.

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
---
 arch/arc/kernel/head.S  |  5 ++++-
 arch/arc/kernel/setup.c | 26 ++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
index 7095055bb874..3fb88ec62bc7 100644
--- a/arch/arc/kernel/head.S
+++ b/arch/arc/kernel/head.S
@@ -92,10 +92,13 @@ ENTRY(stext)
 
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
 	; Uboot - kernel ABI
+	;    r1 = bits [31:8] magic number, bits [7:0] uboot-kernel ABI revision
+	; ABI revision 0:
 	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
-	;    r1 = magic number (board identity, unused as of now
 	;    r2 = pointer to uboot provided cmdline or external DTB in mem
 	; These are handled later in handle_uboot_args()
+	st      r1, [@uboot_rev_magic]
+	mov	r1, 0			; errase magic from the register
 	st	r0, [@uboot_tag]
 	st	r2, [@uboot_arg]
 #endif
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index 7edb35c26322..868dda3d4b43 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -36,6 +36,7 @@ unsigned int intr_to_DE_cnt;
 
 /* Part of U-boot ABI: see head.S */
 int __initdata uboot_tag;
+int __initdata uboot_rev_magic;
 char __initdata *uboot_arg;
 
 const struct machine_desc *machine_desc;
@@ -467,6 +468,10 @@ static inline bool is_kernel(unsigned long addr)
 	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
 }
 
+#define UBOOT_MAGIC_VALUE		0x567890
+#define UBOOT_MAGIC_GET(x)		(((x) & GENMASK(31, 8)) >> 8)
+#define UBOOT_REVISION_GET(x)		((x) & GENMASK(7, 0))
+
 /* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
 #define UBOOT_REV0P_TAG_NONE		0
 #define UBOOT_REV0P_TAG_CMDLINE		1
@@ -478,6 +483,25 @@ void __init handle_uboot_args(void)
 	bool use_embedded_dtb = true;
 
 #ifdef CONFIG_ARC_UBOOT_SUPPORT
+	/* check that we got correct magic */
+	if (UBOOT_MAGIC_GET(uboot_rev_magic) != UBOOT_MAGIC_VALUE) {
+		pr_warn("Invalid magic '%06lx' is passed from uboot, uboot args ingnored\n",
+			UBOOT_MAGIC_GET(uboot_rev_magic));
+
+		goto ignore_uboot_args;
+	}
+
+	/*
+	 * check that we know this U-boot args ABI revision.
+	 * as for today we only have one revision - '0'.
+	 */
+	if (UBOOT_REVISION_GET(uboot_rev_magic) != 0) {
+		pr_warn("Unknown args revision '%02lx' is passed from uboot, uboot args ingnored\n",
+			UBOOT_REVISION_GET(uboot_rev_magic));
+
+		goto ignore_uboot_args;
+	}
+
 	/* check that we know this tag */
 	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
 	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
@@ -498,6 +522,8 @@ void __init handle_uboot_args(void)
 
 	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
 		append_boot_cmdline = true;
+
+ignore_uboot_args:
 #endif
 
 	if (use_embedded_dtb) {
-- 
2.14.5

Re: [RFC 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
>  * don't allow to pass unknown tag.
>  * try to use external device tree blob only if corresponding tag
>    (TAG_DTB) is set.
>  * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
> 
> While I'm at it refactor U-boot arguments handling code.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  arch/arc/kernel/head.S  |  2 +-
>  arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 8b90d25a15cc..7095055bb874 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -95,7 +95,7 @@ ENTRY(stext)
>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
>  	;    r1 = magic number (board identity, unused as of now
>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
> -	; These are handled later in setup_arch()
> +	; These are handled later in handle_uboot_args()
>  	st	r0, [@uboot_tag]
>  	st	r2, [@uboot_arg]
>  #endif
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index feb90093e6b1..7edb35c26322 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -462,43 +462,64 @@ void setup_processor(void)
>  	arc_chk_core_config();
>  }
>  
> -static inline int is_kernel(unsigned long addr)
> +static inline bool is_kernel(unsigned long addr)
>  {
> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> -		return 1;
> -	return 0;

So even though I wrote it eons ago I was confused myself. We panic if this is 1,
because this addr seems inside kernel's resident image (code/data). So add that
comment maybe.

> +	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>  }
>  
> -void __init setup_arch(char **cmdline_p)
> +/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */

Just call it ABI 0, and we call the new ABI 1.

> +#define UBOOT_REV0P_TAG_NONE		0
> +#define UBOOT_REV0P_TAG_CMDLINE		1
> +#define UBOOT_REV0P_TAG_DTB		2
> +
> +void __init handle_uboot_args(void)
>  {
> +	bool append_boot_cmdline = false;
> +	bool use_embedded_dtb = true;
> +
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> +	/* check that we know this tag */
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_DTB)
> +		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
> +
>  	/* make sure that uboot passed pointer to cmdline/dtb is valid */
> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
>  		panic("Invalid uboot arg\n");
>  
>  	/* See if u-boot passed an external Device Tree blob */
> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
> -	if (!machine_desc)
> +	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
> +		machine_desc = setup_machine_fdt(uboot_arg);
> +
> +		/* external Device Tree blob is invalid - use embedded one */
> +		use_embedded_dtb = !machine_desc;
> +	}
> +
> +	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
> +		append_boot_cmdline = true;
>  #endif
> -	{
> -		/* No, so try the embedded one */
> +
> +	if (use_embedded_dtb) {
>  		machine_desc = setup_machine_fdt(__dtb_start);
>  		if (!machine_desc)
>  			panic("Embedded DT invalid\n");
> +	}
>  
> -		/*
> -		 * If we are here, it is established that @uboot_arg didn't
> -		 * point to DT blob. Instead if u-boot says it is cmdline,
> -		 * append to embedded DT cmdline.
> -		 * setup_machine_fdt() would have populated @boot_command_line
> -		 */
> -		if (uboot_tag == 1) {
> -			/* Ensure a whitespace between the 2 cmdlines */
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -			strlcat(boot_command_line, uboot_arg,
> -				COMMAND_LINE_SIZE);
> -		}
> +	/*
> +	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
> +	 * to embedded DT cmdline.
> +	 */

This comment is useless after the more descriptive variable names.

> +	if (append_boot_cmdline) {
> +		/* Ensure a whitespace between the 2 cmdlines */
> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
>  	}
> +}
> +
> +void __init setup_arch(char **cmdline_p)
> +{
> +	handle_uboot_args();
>  
>  	/* Save unparsed command line copy for /proc/cmdline */
>  	*cmdline_p = boot_command_line;
> 

[RFC 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
>  * don't allow to pass unknown tag.
>  * try to use external device tree blob only if corresponding tag
>    (TAG_DTB) is set.
>  * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
> 
> While I'm at it refactor U-boot arguments handling code.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
> ---
>  arch/arc/kernel/head.S  |  2 +-
>  arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 8b90d25a15cc..7095055bb874 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -95,7 +95,7 @@ ENTRY(stext)
>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
>  	;    r1 = magic number (board identity, unused as of now
>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
> -	; These are handled later in setup_arch()
> +	; These are handled later in handle_uboot_args()
>  	st	r0, [@uboot_tag]
>  	st	r2, [@uboot_arg]
>  #endif
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index feb90093e6b1..7edb35c26322 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -462,43 +462,64 @@ void setup_processor(void)
>  	arc_chk_core_config();
>  }
>  
> -static inline int is_kernel(unsigned long addr)
> +static inline bool is_kernel(unsigned long addr)
>  {
> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> -		return 1;
> -	return 0;

So even though I wrote it eons ago I was confused myself. We panic if this is 1,
because this addr seems inside kernel's resident image (code/data). So add that
comment maybe.

> +	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>  }
>  
> -void __init setup_arch(char **cmdline_p)
> +/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */

Just call it ABI 0, and we call the new ABI 1.

> +#define UBOOT_REV0P_TAG_NONE		0
> +#define UBOOT_REV0P_TAG_CMDLINE		1
> +#define UBOOT_REV0P_TAG_DTB		2
> +
> +void __init handle_uboot_args(void)
>  {
> +	bool append_boot_cmdline = false;
> +	bool use_embedded_dtb = true;
> +
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> +	/* check that we know this tag */
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_DTB)
> +		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
> +
>  	/* make sure that uboot passed pointer to cmdline/dtb is valid */
> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
>  		panic("Invalid uboot arg\n");
>  
>  	/* See if u-boot passed an external Device Tree blob */
> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
> -	if (!machine_desc)
> +	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
> +		machine_desc = setup_machine_fdt(uboot_arg);
> +
> +		/* external Device Tree blob is invalid - use embedded one */
> +		use_embedded_dtb = !machine_desc;
> +	}
> +
> +	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
> +		append_boot_cmdline = true;
>  #endif
> -	{
> -		/* No, so try the embedded one */
> +
> +	if (use_embedded_dtb) {
>  		machine_desc = setup_machine_fdt(__dtb_start);
>  		if (!machine_desc)
>  			panic("Embedded DT invalid\n");
> +	}
>  
> -		/*
> -		 * If we are here, it is established that @uboot_arg didn't
> -		 * point to DT blob. Instead if u-boot says it is cmdline,
> -		 * append to embedded DT cmdline.
> -		 * setup_machine_fdt() would have populated @boot_command_line
> -		 */
> -		if (uboot_tag == 1) {
> -			/* Ensure a whitespace between the 2 cmdlines */
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -			strlcat(boot_command_line, uboot_arg,
> -				COMMAND_LINE_SIZE);
> -		}
> +	/*
> +	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
> +	 * to embedded DT cmdline.
> +	 */

This comment is useless after the more descriptive variable names.

> +	if (append_boot_cmdline) {
> +		/* Ensure a whitespace between the 2 cmdlines */
> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
>  	}
> +}
> +
> +void __init setup_arch(char **cmdline_p)
> +{
> +	handle_uboot_args();
>  
>  	/* Save unparsed command line copy for /proc/cmdline */
>  	*cmdline_p = boot_command_line;
> 

Re: [RFC 2/2] ARC: U-boot: check magic number passed from u-boot

[Vineet Gupta]

On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
> In case of devboards we really often disable bootloader and load
> Linux image in memory via JTAG. In case of using kernel with
> CONFIG_ARC_UBOOT_SUPPORT enabled we may crash as we will try to
> interpret some junk in a registers as a pointers to bootargs/etc
> which aren't set by anyone in case of JTAG using.
> 
> Try to make it much less possible by check magic number and
> 'U-boot - kernel' ABI revision number passed from U-boot.
> Ignore U-boot arguments if we got wrong magic number or unknown
> ABI revision.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  arch/arc/kernel/head.S  |  5 ++++-
>  arch/arc/kernel/setup.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 7095055bb874..3fb88ec62bc7 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -92,10 +92,13 @@ ENTRY(stext)
>  
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
>  	; Uboot - kernel ABI
> +	;    r1 = bits [31:8] magic number, bits [7:0] uboot-kernel ABI revision
> +	; ABI revision 0:
>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
> -	;    r1 = magic number (board identity, unused as of now
>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
>  	; These are handled later in handle_uboot_args()
> +	st      r1, [@uboot_rev_magic]
> +	mov	r1, 0			; errase magic from the register
>  	st	r0, [@uboot_tag]
>  	st	r2, [@uboot_arg]
>  #endif
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index 7edb35c26322..868dda3d4b43 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -36,6 +36,7 @@ unsigned int intr_to_DE_cnt;
>  
>  /* Part of U-boot ABI: see head.S */
>  int __initdata uboot_tag;
> +int __initdata uboot_rev_magic;
>  char __initdata *uboot_arg;
>  
>  const struct machine_desc *machine_desc;
> @@ -467,6 +468,10 @@ static inline bool is_kernel(unsigned long addr)
>  	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>  }
>  
> +#define UBOOT_MAGIC_VALUE		0x567890
> +#define UBOOT_MAGIC_GET(x)		(((x) & GENMASK(31, 8)) >> 8)
> +#define UBOOT_REVISION_GET(x)		((x) & GENMASK(7, 0))
> +
>  /* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
>  #define UBOOT_REV0P_TAG_NONE		0
>  #define UBOOT_REV0P_TAG_CMDLINE		1
> @@ -478,6 +483,25 @@ void __init handle_uboot_args(void)
>  	bool use_embedded_dtb = true;
>  
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> +	/* check that we got correct magic */
> +	if (UBOOT_MAGIC_GET(uboot_rev_magic) != UBOOT_MAGIC_VALUE) {
> +		pr_warn("Invalid magic '%06lx' is passed from uboot, uboot args ingnored\n",
> +			UBOOT_MAGIC_GET(uboot_rev_magic));
> +
> +		goto ignore_uboot_args;
> +	}
> +
> +	/*
> +	 * check that we know this U-boot args ABI revision.
> +	 * as for today we only have one revision - '0'.
> +	 */
> +	if (UBOOT_REVISION_GET(uboot_rev_magic) != 0) {
> +		pr_warn("Unknown args revision '%02lx' is passed from uboot, uboot args ingnored\n",
> +			UBOOT_REVISION_GET(uboot_rev_magic));
> +
> +		goto ignore_uboot_args;
> +	}
> +

So you are effectively dropping support for older uboot here as above seems to
assume that r1 will have the new magic value now. The existing stock of HSDK
boards will continue to ship with older uboot.

I understand your idea, but the fact is we can't drop the old uboot support.

Also not sure how all of this allows us to eliminate CONFIG_ARC_UBOOT_SUPPORT, or
enable it unconditionally with old or new uboot.


>  	/* check that we know this tag */
>  	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
>  	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
> @@ -498,6 +522,8 @@ void __init handle_uboot_args(void)
>  
>  	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
>  		append_boot_cmdline = true;
> +
> +ignore_uboot_args:
>  #endif
>  
>  	if (use_embedded_dtb) {
> 

[RFC 2/2] ARC: U-boot: check magic number passed from u-boot

[Vineet Gupta]

On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
> In case of devboards we really often disable bootloader and load
> Linux image in memory via JTAG. In case of using kernel with
> CONFIG_ARC_UBOOT_SUPPORT enabled we may crash as we will try to
> interpret some junk in a registers as a pointers to bootargs/etc
> which aren't set by anyone in case of JTAG using.
> 
> Try to make it much less possible by check magic number and
> 'U-boot - kernel' ABI revision number passed from U-boot.
> Ignore U-boot arguments if we got wrong magic number or unknown
> ABI revision.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
> ---
>  arch/arc/kernel/head.S  |  5 ++++-
>  arch/arc/kernel/setup.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 7095055bb874..3fb88ec62bc7 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -92,10 +92,13 @@ ENTRY(stext)
>  
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
>  	; Uboot - kernel ABI
> +	;    r1 = bits [31:8] magic number, bits [7:0] uboot-kernel ABI revision
> +	; ABI revision 0:
>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
> -	;    r1 = magic number (board identity, unused as of now
>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
>  	; These are handled later in handle_uboot_args()
> +	st      r1, [@uboot_rev_magic]
> +	mov	r1, 0			; errase magic from the register
>  	st	r0, [@uboot_tag]
>  	st	r2, [@uboot_arg]
>  #endif
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index 7edb35c26322..868dda3d4b43 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -36,6 +36,7 @@ unsigned int intr_to_DE_cnt;
>  
>  /* Part of U-boot ABI: see head.S */
>  int __initdata uboot_tag;
> +int __initdata uboot_rev_magic;
>  char __initdata *uboot_arg;
>  
>  const struct machine_desc *machine_desc;
> @@ -467,6 +468,10 @@ static inline bool is_kernel(unsigned long addr)
>  	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>  }
>  
> +#define UBOOT_MAGIC_VALUE		0x567890
> +#define UBOOT_MAGIC_GET(x)		(((x) & GENMASK(31, 8)) >> 8)
> +#define UBOOT_REVISION_GET(x)		((x) & GENMASK(7, 0))
> +
>  /* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
>  #define UBOOT_REV0P_TAG_NONE		0
>  #define UBOOT_REV0P_TAG_CMDLINE		1
> @@ -478,6 +483,25 @@ void __init handle_uboot_args(void)
>  	bool use_embedded_dtb = true;
>  
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> +	/* check that we got correct magic */
> +	if (UBOOT_MAGIC_GET(uboot_rev_magic) != UBOOT_MAGIC_VALUE) {
> +		pr_warn("Invalid magic '%06lx' is passed from uboot, uboot args ingnored\n",
> +			UBOOT_MAGIC_GET(uboot_rev_magic));
> +
> +		goto ignore_uboot_args;
> +	}
> +
> +	/*
> +	 * check that we know this U-boot args ABI revision.
> +	 * as for today we only have one revision - '0'.
> +	 */
> +	if (UBOOT_REVISION_GET(uboot_rev_magic) != 0) {
> +		pr_warn("Unknown args revision '%02lx' is passed from uboot, uboot args ingnored\n",
> +			UBOOT_REVISION_GET(uboot_rev_magic));
> +
> +		goto ignore_uboot_args;
> +	}
> +

So you are effectively dropping support for older uboot here as above seems to
assume that r1 will have the new magic value now. The existing stock of HSDK
boards will continue to ship with older uboot.

I understand your idea, but the fact is we can't drop the old uboot support.

Also not sure how all of this allows us to eliminate CONFIG_ARC_UBOOT_SUPPORT, or
enable it unconditionally with old or new uboot.


>  	/* check that we know this tag */
>  	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
>  	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
> @@ -498,6 +522,8 @@ void __init handle_uboot_args(void)
>  
>  	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
>  		append_boot_cmdline = true;
> +
> +ignore_uboot_args:
>  #endif
>  
>  	if (use_embedded_dtb) {
> 

Re: [RFC 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
>  * don't allow to pass unknown tag.
>  * try to use external device tree blob only if corresponding tag
>    (TAG_DTB) is set.
>  * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
> 
> While I'm at it refactor U-boot arguments handling code.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
>  arch/arc/kernel/head.S  |  2 +-
>  arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 8b90d25a15cc..7095055bb874 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -95,7 +95,7 @@ ENTRY(stext)
>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
>  	;    r1 = magic number (board identity, unused as of now
>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
> -	; These are handled later in setup_arch()
> +	; These are handled later in handle_uboot_args()
>  	st	r0, [@uboot_tag]
>  	st	r2, [@uboot_arg]
>  #endif
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index feb90093e6b1..7edb35c26322 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -462,43 +462,64 @@ void setup_processor(void)
>  	arc_chk_core_config();
>  }
>  
> -static inline int is_kernel(unsigned long addr)
> +static inline bool is_kernel(unsigned long addr)
>  {
> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> -		return 1;
> -	return 0;
> +	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>  }
>  
> -void __init setup_arch(char **cmdline_p)
> +/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
> +#define UBOOT_REV0P_TAG_NONE		0
> +#define UBOOT_REV0P_TAG_CMDLINE		1
> +#define UBOOT_REV0P_TAG_DTB		2
> +
> +void __init handle_uboot_args(void)
>  {
> +	bool append_boot_cmdline = false;
> +	bool use_embedded_dtb = true;
> +
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> +	/* check that we know this tag */
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_DTB)
> +		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
> +
>  	/* make sure that uboot passed pointer to cmdline/dtb is valid */
> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
>  		panic("Invalid uboot arg\n");
>  
>  	/* See if u-boot passed an external Device Tree blob */
> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
> -	if (!machine_desc)
> +	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
> +		machine_desc = setup_machine_fdt(uboot_arg);
> +
> +		/* external Device Tree blob is invalid - use embedded one */
> +		use_embedded_dtb = !machine_desc;
> +	}
> +
> +	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
> +		append_boot_cmdline = true;
>  #endif
> -	{
> -		/* No, so try the embedded one */
> +
> +	if (use_embedded_dtb) {
>  		machine_desc = setup_machine_fdt(__dtb_start);
>  		if (!machine_desc)
>  			panic("Embedded DT invalid\n");
> +	}
>  
> -		/*
> -		 * If we are here, it is established that @uboot_arg didn't
> -		 * point to DT blob. Instead if u-boot says it is cmdline,
> -		 * append to embedded DT cmdline.
> -		 * setup_machine_fdt() would have populated @boot_command_line
> -		 */
> -		if (uboot_tag == 1) {
> -			/* Ensure a whitespace between the 2 cmdlines */
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -			strlcat(boot_command_line, uboot_arg,
> -				COMMAND_LINE_SIZE);
> -		}
> +	/*
> +	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
> +	 * to embedded DT cmdline.
> +	 */
> +	if (append_boot_cmdline) {
> +		/* Ensure a whitespace between the 2 cmdlines */
> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
>  	}
> +}
> +
> +void __init setup_arch(char **cmdline_p)
> +{
> +	handle_uboot_args();
>  
>  	/* Save unparsed command line copy for /proc/cmdline */
>  	*cmdline_p = boot_command_line;

I think we can grossly simplify all of this w/o adding any new ABI contract
between kernel and uboot and eliminate CONFIG_ARC_UBOOT_SUPPORT as well (make
uboot support always enabled)

So when bootloader runs it passes {0,1,2} in r0 and corresponding arg in r2.
For jtag case we can assume that core registers will come up reset value of 0 or
in worst case we rely on user passing -on=clear_regs to Metaware debugger.

Now as you already figured out, we just need to make sure kernel doesn't try to
dereference the pointers for bogus values. How does the hunk below look like (and
in a subsequent patch remove the Kconfig)

-------------->
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index def19b0ef8c6..cdd8e9a1768a 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -462,44 +462,46 @@ void setup_processor(void)
 	arc_chk_core_config();
 }

-static inline int is_kernel(unsigned long addr)
-{
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
-		return 1;
-	return 0;
-}
-
 void __init setup_arch(char **cmdline_p)
 {
-#ifdef CONFIG_ARC_UBOOT_SUPPORT
-	/* make sure that uboot passed pointer to cmdline/dtb is valid */
-	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
-		panic("Invalid uboot arg\n");
-
-	/* See if u-boot passed an external Device Tree blob */
-	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
-	if (!machine_desc)
-#endif
-	{
-		/* No, so try the embedded one */
-		machine_desc = setup_machine_fdt(__dtb_start);
-		if (!machine_desc)
-			panic("Embedded DT invalid\n");
+	bool use_embedded_dtb = true;
+
+	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag) {

 		/*
-		 * If we are here, it is established that @uboot_arg didn't
-		 * point to DT blob. Instead if u-boot says it is cmdline,
-		 * append to embedded DT cmdline.
-		 * setup_machine_fdt() would have populated @boot_command_line
+		 * ensure u-boot passed pointer is valid
+		 *   - is a valid untranslated address (although MMU is not
+		 *     enabled yet, it being a high address ensures this is
+		 *     not by fluke)
+		 *   - doesn't clobber resident kernel image
 		 */
-		if (uboot_tag == 1) {
-			/* Ensure a whitespace between the 2 cmdlines */
-			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
-			strlcat(boot_command_line, uboot_arg,
-				COMMAND_LINE_SIZE);
+		if ((unsigned long)uboot_arg < (unsigned long)_end)
+			panic("Invalid uboot arg\n");
+
+		/* validate u-boot passed external Device Tree blob */
+		if (uboot_tag == 2) {
+			machine_desc = setup_machine_fdt(uboot_arg);
+			if (machine_desc)
+				use_embedded_dtb = false;
 		}
 	}

+	if (use_embedded_dtb) 	{
+		machine_desc = setup_machine_fdt(__dtb_start);
+		if (!machine_desc)
+			panic("Embedded DT invalid\n");
+	}
+
+	/*
+	 * append u-boot cmdline to embedded DT cmdline.
+	 * setup_machine_fdt() would have populated @boot_command_line
+	 */
+	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag == 1) {
+		/* Ensure a whitespace between the 2 cmdlines */
+		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
+	}
+
 	/* Save unparsed command line copy for /proc/cmdline */
 	*cmdline_p = boot_command_line;

-- 
2.7.4

[RFC 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
> Handle U-boot arguments paranoidly:
>  * don't allow to pass unknown tag.
>  * try to use external device tree blob only if corresponding tag
>    (TAG_DTB) is set.
>  * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
> 
> While I'm at it refactor U-boot arguments handling code.
> 
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
> ---
>  arch/arc/kernel/head.S  |  2 +-
>  arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 44 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
> index 8b90d25a15cc..7095055bb874 100644
> --- a/arch/arc/kernel/head.S
> +++ b/arch/arc/kernel/head.S
> @@ -95,7 +95,7 @@ ENTRY(stext)
>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
>  	;    r1 = magic number (board identity, unused as of now
>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
> -	; These are handled later in setup_arch()
> +	; These are handled later in handle_uboot_args()
>  	st	r0, [@uboot_tag]
>  	st	r2, [@uboot_arg]
>  #endif
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index feb90093e6b1..7edb35c26322 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -462,43 +462,64 @@ void setup_processor(void)
>  	arc_chk_core_config();
>  }
>  
> -static inline int is_kernel(unsigned long addr)
> +static inline bool is_kernel(unsigned long addr)
>  {
> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> -		return 1;
> -	return 0;
> +	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>  }
>  
> -void __init setup_arch(char **cmdline_p)
> +/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
> +#define UBOOT_REV0P_TAG_NONE		0
> +#define UBOOT_REV0P_TAG_CMDLINE		1
> +#define UBOOT_REV0P_TAG_DTB		2
> +
> +void __init handle_uboot_args(void)
>  {
> +	bool append_boot_cmdline = false;
> +	bool use_embedded_dtb = true;
> +
>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
> +	/* check that we know this tag */
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
> +	    uboot_tag != UBOOT_REV0P_TAG_DTB)
> +		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
> +
>  	/* make sure that uboot passed pointer to cmdline/dtb is valid */
> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
>  		panic("Invalid uboot arg\n");
>  
>  	/* See if u-boot passed an external Device Tree blob */
> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
> -	if (!machine_desc)
> +	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
> +		machine_desc = setup_machine_fdt(uboot_arg);
> +
> +		/* external Device Tree blob is invalid - use embedded one */
> +		use_embedded_dtb = !machine_desc;
> +	}
> +
> +	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
> +		append_boot_cmdline = true;
>  #endif
> -	{
> -		/* No, so try the embedded one */
> +
> +	if (use_embedded_dtb) {
>  		machine_desc = setup_machine_fdt(__dtb_start);
>  		if (!machine_desc)
>  			panic("Embedded DT invalid\n");
> +	}
>  
> -		/*
> -		 * If we are here, it is established that @uboot_arg didn't
> -		 * point to DT blob. Instead if u-boot says it is cmdline,
> -		 * append to embedded DT cmdline.
> -		 * setup_machine_fdt() would have populated @boot_command_line
> -		 */
> -		if (uboot_tag == 1) {
> -			/* Ensure a whitespace between the 2 cmdlines */
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -			strlcat(boot_command_line, uboot_arg,
> -				COMMAND_LINE_SIZE);
> -		}
> +	/*
> +	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
> +	 * to embedded DT cmdline.
> +	 */
> +	if (append_boot_cmdline) {
> +		/* Ensure a whitespace between the 2 cmdlines */
> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
>  	}
> +}
> +
> +void __init setup_arch(char **cmdline_p)
> +{
> +	handle_uboot_args();
>  
>  	/* Save unparsed command line copy for /proc/cmdline */
>  	*cmdline_p = boot_command_line;

I think we can grossly simplify all of this w/o adding any new ABI contract
between kernel and uboot and eliminate CONFIG_ARC_UBOOT_SUPPORT as well (make
uboot support always enabled)

So when bootloader runs it passes {0,1,2} in r0 and corresponding arg in r2.
For jtag case we can assume that core registers will come up reset value of 0 or
in worst case we rely on user passing -on=clear_regs to Metaware debugger.

Now as you already figured out, we just need to make sure kernel doesn't try to
dereference the pointers for bogus values. How does the hunk below look like (and
in a subsequent patch remove the Kconfig)

-------------->
diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
index def19b0ef8c6..cdd8e9a1768a 100644
--- a/arch/arc/kernel/setup.c
+++ b/arch/arc/kernel/setup.c
@@ -462,44 +462,46 @@ void setup_processor(void)
 	arc_chk_core_config();
 }

-static inline int is_kernel(unsigned long addr)
-{
-	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
-		return 1;
-	return 0;
-}
-
 void __init setup_arch(char **cmdline_p)
 {
-#ifdef CONFIG_ARC_UBOOT_SUPPORT
-	/* make sure that uboot passed pointer to cmdline/dtb is valid */
-	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
-		panic("Invalid uboot arg\n");
-
-	/* See if u-boot passed an external Device Tree blob */
-	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
-	if (!machine_desc)
-#endif
-	{
-		/* No, so try the embedded one */
-		machine_desc = setup_machine_fdt(__dtb_start);
-		if (!machine_desc)
-			panic("Embedded DT invalid\n");
+	bool use_embedded_dtb = true;
+
+	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag) {

 		/*
-		 * If we are here, it is established that @uboot_arg didn't
-		 * point to DT blob. Instead if u-boot says it is cmdline,
-		 * append to embedded DT cmdline.
-		 * setup_machine_fdt() would have populated @boot_command_line
+		 * ensure u-boot passed pointer is valid
+		 *   - is a valid untranslated address (although MMU is not
+		 *     enabled yet, it being a high address ensures this is
+		 *     not by fluke)
+		 *   - doesn't clobber resident kernel image
 		 */
-		if (uboot_tag == 1) {
-			/* Ensure a whitespace between the 2 cmdlines */
-			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
-			strlcat(boot_command_line, uboot_arg,
-				COMMAND_LINE_SIZE);
+		if ((unsigned long)uboot_arg < (unsigned long)_end)
+			panic("Invalid uboot arg\n");
+
+		/* validate u-boot passed external Device Tree blob */
+		if (uboot_tag == 2) {
+			machine_desc = setup_machine_fdt(uboot_arg);
+			if (machine_desc)
+				use_embedded_dtb = false;
 		}
 	}

+	if (use_embedded_dtb) 	{
+		machine_desc = setup_machine_fdt(__dtb_start);
+		if (!machine_desc)
+			panic("Embedded DT invalid\n");
+	}
+
+	/*
+	 * append u-boot cmdline to embedded DT cmdline.
+	 * setup_machine_fdt() would have populated @boot_command_line
+	 */
+	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag == 1) {
+		/* Ensure a whitespace between the 2 cmdlines */
+		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
+		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
+	}
+
 	/* Save unparsed command line copy for /proc/cmdline */
 	*cmdline_p = boot_command_line;

-- 
2.7.4

Re: [RFC 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

Ping ! Are you happy with approach ?

-Vineet

On 2/6/19 2:13 PM, Vineet Gupta wrote:
> On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
>> Handle U-boot arguments paranoidly:
>>  * don't allow to pass unknown tag.
>>  * try to use external device tree blob only if corresponding tag
>>    (TAG_DTB) is set.
>>  * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
>>
>> While I'm at it refactor U-boot arguments handling code.
>>
>> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
>> ---
>>  arch/arc/kernel/head.S  |  2 +-
>>  arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
>>  2 files changed, 44 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
>> index 8b90d25a15cc..7095055bb874 100644
>> --- a/arch/arc/kernel/head.S
>> +++ b/arch/arc/kernel/head.S
>> @@ -95,7 +95,7 @@ ENTRY(stext)
>>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
>>  	;    r1 = magic number (board identity, unused as of now
>>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
>> -	; These are handled later in setup_arch()
>> +	; These are handled later in handle_uboot_args()
>>  	st	r0, [@uboot_tag]
>>  	st	r2, [@uboot_arg]
>>  #endif
>> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
>> index feb90093e6b1..7edb35c26322 100644
>> --- a/arch/arc/kernel/setup.c
>> +++ b/arch/arc/kernel/setup.c
>> @@ -462,43 +462,64 @@ void setup_processor(void)
>>  	arc_chk_core_config();
>>  }
>>  
>> -static inline int is_kernel(unsigned long addr)
>> +static inline bool is_kernel(unsigned long addr)
>>  {
>> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
>> -		return 1;
>> -	return 0;
>> +	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>>  }
>>  
>> -void __init setup_arch(char **cmdline_p)
>> +/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
>> +#define UBOOT_REV0P_TAG_NONE		0
>> +#define UBOOT_REV0P_TAG_CMDLINE		1
>> +#define UBOOT_REV0P_TAG_DTB		2
>> +
>> +void __init handle_uboot_args(void)
>>  {
>> +	bool append_boot_cmdline = false;
>> +	bool use_embedded_dtb = true;
>> +
>>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
>> +	/* check that we know this tag */
>> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
>> +	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
>> +	    uboot_tag != UBOOT_REV0P_TAG_DTB)
>> +		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
>> +
>>  	/* make sure that uboot passed pointer to cmdline/dtb is valid */
>> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
>> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
>>  		panic("Invalid uboot arg\n");
>>  
>>  	/* See if u-boot passed an external Device Tree blob */
>> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
>> -	if (!machine_desc)
>> +	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
>> +		machine_desc = setup_machine_fdt(uboot_arg);
>> +
>> +		/* external Device Tree blob is invalid - use embedded one */
>> +		use_embedded_dtb = !machine_desc;
>> +	}
>> +
>> +	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
>> +		append_boot_cmdline = true;
>>  #endif
>> -	{
>> -		/* No, so try the embedded one */
>> +
>> +	if (use_embedded_dtb) {
>>  		machine_desc = setup_machine_fdt(__dtb_start);
>>  		if (!machine_desc)
>>  			panic("Embedded DT invalid\n");
>> +	}
>>  
>> -		/*
>> -		 * If we are here, it is established that @uboot_arg didn't
>> -		 * point to DT blob. Instead if u-boot says it is cmdline,
>> -		 * append to embedded DT cmdline.
>> -		 * setup_machine_fdt() would have populated @boot_command_line
>> -		 */
>> -		if (uboot_tag == 1) {
>> -			/* Ensure a whitespace between the 2 cmdlines */
>> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>> -			strlcat(boot_command_line, uboot_arg,
>> -				COMMAND_LINE_SIZE);
>> -		}
>> +	/*
>> +	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
>> +	 * to embedded DT cmdline.
>> +	 */
>> +	if (append_boot_cmdline) {
>> +		/* Ensure a whitespace between the 2 cmdlines */
>> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
>>  	}
>> +}
>> +
>> +void __init setup_arch(char **cmdline_p)
>> +{
>> +	handle_uboot_args();
>>  
>>  	/* Save unparsed command line copy for /proc/cmdline */
>>  	*cmdline_p = boot_command_line;
> 
> I think we can grossly simplify all of this w/o adding any new ABI contract
> between kernel and uboot and eliminate CONFIG_ARC_UBOOT_SUPPORT as well (make
> uboot support always enabled)
> 
> So when bootloader runs it passes {0,1,2} in r0 and corresponding arg in r2.
> For jtag case we can assume that core registers will come up reset value of 0 or
> in worst case we rely on user passing -on=clear_regs to Metaware debugger.
> 
> Now as you already figured out, we just need to make sure kernel doesn't try to
> dereference the pointers for bogus values. How does the hunk below look like (and
> in a subsequent patch remove the Kconfig)
> 
> -------------->
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index def19b0ef8c6..cdd8e9a1768a 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -462,44 +462,46 @@ void setup_processor(void)
>  	arc_chk_core_config();
>  }
> 
> -static inline int is_kernel(unsigned long addr)
> -{
> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> -		return 1;
> -	return 0;
> -}
> -
>  void __init setup_arch(char **cmdline_p)
>  {
> -#ifdef CONFIG_ARC_UBOOT_SUPPORT
> -	/* make sure that uboot passed pointer to cmdline/dtb is valid */
> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> -		panic("Invalid uboot arg\n");
> -
> -	/* See if u-boot passed an external Device Tree blob */
> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
> -	if (!machine_desc)
> -#endif
> -	{
> -		/* No, so try the embedded one */
> -		machine_desc = setup_machine_fdt(__dtb_start);
> -		if (!machine_desc)
> -			panic("Embedded DT invalid\n");
> +	bool use_embedded_dtb = true;
> +
> +	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag) {
> 
>  		/*
> -		 * If we are here, it is established that @uboot_arg didn't
> -		 * point to DT blob. Instead if u-boot says it is cmdline,
> -		 * append to embedded DT cmdline.
> -		 * setup_machine_fdt() would have populated @boot_command_line
> +		 * ensure u-boot passed pointer is valid
> +		 *   - is a valid untranslated address (although MMU is not
> +		 *     enabled yet, it being a high address ensures this is
> +		 *     not by fluke)
> +		 *   - doesn't clobber resident kernel image
>  		 */
> -		if (uboot_tag == 1) {
> -			/* Ensure a whitespace between the 2 cmdlines */
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -			strlcat(boot_command_line, uboot_arg,
> -				COMMAND_LINE_SIZE);
> +		if ((unsigned long)uboot_arg < (unsigned long)_end)
> +			panic("Invalid uboot arg\n");
> +
> +		/* validate u-boot passed external Device Tree blob */
> +		if (uboot_tag == 2) {
> +			machine_desc = setup_machine_fdt(uboot_arg);
> +			if (machine_desc)
> +				use_embedded_dtb = false;
>  		}
>  	}
> 
> +	if (use_embedded_dtb) 	{
> +		machine_desc = setup_machine_fdt(__dtb_start);
> +		if (!machine_desc)
> +			panic("Embedded DT invalid\n");
> +	}
> +
> +	/*
> +	 * append u-boot cmdline to embedded DT cmdline.
> +	 * setup_machine_fdt() would have populated @boot_command_line
> +	 */
> +	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag == 1) {
> +		/* Ensure a whitespace between the 2 cmdlines */
> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
> +	}
> +
>  	/* Save unparsed command line copy for /proc/cmdline */
>  	*cmdline_p = boot_command_line;
> 

[RFC 1/2] ARC: U-boot: check arguments paranoidly

[Vineet Gupta]

Ping ! Are you happy with approach ?

-Vineet

On 2/6/19 2:13 PM, Vineet Gupta wrote:
> On 2/6/19 9:22 AM, Eugeniy Paltsev wrote:
>> Handle U-boot arguments paranoidly:
>>  * don't allow to pass unknown tag.
>>  * try to use external device tree blob only if corresponding tag
>>    (TAG_DTB) is set.
>>  * don't check: uboot_tag if kernel build with no ARC_UBOOT_SUPPORT.
>>
>> While I'm at it refactor U-boot arguments handling code.
>>
>> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev at synopsys.com>
>> ---
>>  arch/arc/kernel/head.S  |  2 +-
>>  arch/arc/kernel/setup.c | 65 ++++++++++++++++++++++++++++++++-----------------
>>  2 files changed, 44 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/arc/kernel/head.S b/arch/arc/kernel/head.S
>> index 8b90d25a15cc..7095055bb874 100644
>> --- a/arch/arc/kernel/head.S
>> +++ b/arch/arc/kernel/head.S
>> @@ -95,7 +95,7 @@ ENTRY(stext)
>>  	;    r0 = [0] No uboot interaction, [1] cmdline in r2, [2] DTB in r2
>>  	;    r1 = magic number (board identity, unused as of now
>>  	;    r2 = pointer to uboot provided cmdline or external DTB in mem
>> -	; These are handled later in setup_arch()
>> +	; These are handled later in handle_uboot_args()
>>  	st	r0, [@uboot_tag]
>>  	st	r2, [@uboot_arg]
>>  #endif
>> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
>> index feb90093e6b1..7edb35c26322 100644
>> --- a/arch/arc/kernel/setup.c
>> +++ b/arch/arc/kernel/setup.c
>> @@ -462,43 +462,64 @@ void setup_processor(void)
>>  	arc_chk_core_config();
>>  }
>>  
>> -static inline int is_kernel(unsigned long addr)
>> +static inline bool is_kernel(unsigned long addr)
>>  {
>> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
>> -		return 1;
>> -	return 0;
>> +	return addr >= (unsigned long)_stext && addr <= (unsigned long)_end;
>>  }
>>  
>> -void __init setup_arch(char **cmdline_p)
>> +/* uboot_tag values for U-boot - kernel ABI revisions 0+; see head.S */
>> +#define UBOOT_REV0P_TAG_NONE		0
>> +#define UBOOT_REV0P_TAG_CMDLINE		1
>> +#define UBOOT_REV0P_TAG_DTB		2
>> +
>> +void __init handle_uboot_args(void)
>>  {
>> +	bool append_boot_cmdline = false;
>> +	bool use_embedded_dtb = true;
>> +
>>  #ifdef CONFIG_ARC_UBOOT_SUPPORT
>> +	/* check that we know this tag */
>> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE &&
>> +	    uboot_tag != UBOOT_REV0P_TAG_CMDLINE &&
>> +	    uboot_tag != UBOOT_REV0P_TAG_DTB)
>> +		panic("Invalid uboot tag: '%08x'\n", uboot_tag);
>> +
>>  	/* make sure that uboot passed pointer to cmdline/dtb is valid */
>> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
>> +	if (uboot_tag != UBOOT_REV0P_TAG_NONE && is_kernel((unsigned long)uboot_arg))
>>  		panic("Invalid uboot arg\n");
>>  
>>  	/* See if u-boot passed an external Device Tree blob */
>> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
>> -	if (!machine_desc)
>> +	if (uboot_tag == UBOOT_REV0P_TAG_DTB) {
>> +		machine_desc = setup_machine_fdt(uboot_arg);
>> +
>> +		/* external Device Tree blob is invalid - use embedded one */
>> +		use_embedded_dtb = !machine_desc;
>> +	}
>> +
>> +	if (uboot_tag == UBOOT_REV0P_TAG_CMDLINE)
>> +		append_boot_cmdline = true;
>>  #endif
>> -	{
>> -		/* No, so try the embedded one */
>> +
>> +	if (use_embedded_dtb) {
>>  		machine_desc = setup_machine_fdt(__dtb_start);
>>  		if (!machine_desc)
>>  			panic("Embedded DT invalid\n");
>> +	}
>>  
>> -		/*
>> -		 * If we are here, it is established that @uboot_arg didn't
>> -		 * point to DT blob. Instead if u-boot says it is cmdline,
>> -		 * append to embedded DT cmdline.
>> -		 * setup_machine_fdt() would have populated @boot_command_line
>> -		 */
>> -		if (uboot_tag == 1) {
>> -			/* Ensure a whitespace between the 2 cmdlines */
>> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>> -			strlcat(boot_command_line, uboot_arg,
>> -				COMMAND_LINE_SIZE);
>> -		}
>> +	/*
>> +	 * If we are here, U-boot says that @uboot_arg is cmdline, so append it
>> +	 * to embedded DT cmdline.
>> +	 */
>> +	if (append_boot_cmdline) {
>> +		/* Ensure a whitespace between the 2 cmdlines */
>> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
>> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
>>  	}
>> +}
>> +
>> +void __init setup_arch(char **cmdline_p)
>> +{
>> +	handle_uboot_args();
>>  
>>  	/* Save unparsed command line copy for /proc/cmdline */
>>  	*cmdline_p = boot_command_line;
> 
> I think we can grossly simplify all of this w/o adding any new ABI contract
> between kernel and uboot and eliminate CONFIG_ARC_UBOOT_SUPPORT as well (make
> uboot support always enabled)
> 
> So when bootloader runs it passes {0,1,2} in r0 and corresponding arg in r2.
> For jtag case we can assume that core registers will come up reset value of 0 or
> in worst case we rely on user passing -on=clear_regs to Metaware debugger.
> 
> Now as you already figured out, we just need to make sure kernel doesn't try to
> dereference the pointers for bogus values. How does the hunk below look like (and
> in a subsequent patch remove the Kconfig)
> 
> -------------->
> diff --git a/arch/arc/kernel/setup.c b/arch/arc/kernel/setup.c
> index def19b0ef8c6..cdd8e9a1768a 100644
> --- a/arch/arc/kernel/setup.c
> +++ b/arch/arc/kernel/setup.c
> @@ -462,44 +462,46 @@ void setup_processor(void)
>  	arc_chk_core_config();
>  }
> 
> -static inline int is_kernel(unsigned long addr)
> -{
> -	if (addr >= (unsigned long)_stext && addr <= (unsigned long)_end)
> -		return 1;
> -	return 0;
> -}
> -
>  void __init setup_arch(char **cmdline_p)
>  {
> -#ifdef CONFIG_ARC_UBOOT_SUPPORT
> -	/* make sure that uboot passed pointer to cmdline/dtb is valid */
> -	if (uboot_tag && is_kernel((unsigned long)uboot_arg))
> -		panic("Invalid uboot arg\n");
> -
> -	/* See if u-boot passed an external Device Tree blob */
> -	machine_desc = setup_machine_fdt(uboot_arg);	/* uboot_tag == 2 */
> -	if (!machine_desc)
> -#endif
> -	{
> -		/* No, so try the embedded one */
> -		machine_desc = setup_machine_fdt(__dtb_start);
> -		if (!machine_desc)
> -			panic("Embedded DT invalid\n");
> +	bool use_embedded_dtb = true;
> +
> +	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag) {
> 
>  		/*
> -		 * If we are here, it is established that @uboot_arg didn't
> -		 * point to DT blob. Instead if u-boot says it is cmdline,
> -		 * append to embedded DT cmdline.
> -		 * setup_machine_fdt() would have populated @boot_command_line
> +		 * ensure u-boot passed pointer is valid
> +		 *   - is a valid untranslated address (although MMU is not
> +		 *     enabled yet, it being a high address ensures this is
> +		 *     not by fluke)
> +		 *   - doesn't clobber resident kernel image
>  		 */
> -		if (uboot_tag == 1) {
> -			/* Ensure a whitespace between the 2 cmdlines */
> -			strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> -			strlcat(boot_command_line, uboot_arg,
> -				COMMAND_LINE_SIZE);
> +		if ((unsigned long)uboot_arg < (unsigned long)_end)
> +			panic("Invalid uboot arg\n");
> +
> +		/* validate u-boot passed external Device Tree blob */
> +		if (uboot_tag == 2) {
> +			machine_desc = setup_machine_fdt(uboot_arg);
> +			if (machine_desc)
> +				use_embedded_dtb = false;
>  		}
>  	}
> 
> +	if (use_embedded_dtb) 	{
> +		machine_desc = setup_machine_fdt(__dtb_start);
> +		if (!machine_desc)
> +			panic("Embedded DT invalid\n");
> +	}
> +
> +	/*
> +	 * append u-boot cmdline to embedded DT cmdline.
> +	 * setup_machine_fdt() would have populated @boot_command_line
> +	 */
> +	if (IS_ENABLED(CONFIG_ARC_UBOOT_SUPPORT) && uboot_tag == 1) {
> +		/* Ensure a whitespace between the 2 cmdlines */
> +		strlcat(boot_command_line, " ", COMMAND_LINE_SIZE);
> +		strlcat(boot_command_line, uboot_arg, COMMAND_LINE_SIZE);
> +	}
> +
>  	/* Save unparsed command line copy for /proc/cmdline */
>  	*cmdline_p = boot_command_line;
>