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@xxxxxxxxxxxx>
>> ---
>> 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;
>
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
