[PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Bjørn Mork]

Boards based on the same SoC family can use different boot loaders.
These may pass numeric arguments which we erroneously interpret as
command line or environment pointers. Such errors will cause boot
to halt at an early stage since commit 056a68cea01e ("mips: allow
firmware to pass RNG seed to kernel").

One known example of this issue is a HPE switch using a BootWare
boot loader.  It was found to pass these arguments to the kernel:

  0x00020000 0x00060000 0xfffdffff 0x0000416c

We can avoid hanging by validating that both passed pointers are in
KSEG1 as expected.

Cc: stable@vger.kernel.org
Fixes: 14aecdd41921 ("MIPS: FW: Add environment variable processing.")
Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 arch/mips/fw/lib/cmdline.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/fw/lib/cmdline.c b/arch/mips/fw/lib/cmdline.c
index 892765b742bb..51238c4f9455 100644
--- a/arch/mips/fw/lib/cmdline.c
+++ b/arch/mips/fw/lib/cmdline.c
@@ -22,7 +22,7 @@ void __init fw_init_cmdline(void)
 	int i;
 
 	/* Validate command line parameters. */
-	if ((fw_arg0 >= CKSEG0) || (fw_arg1 < CKSEG0)) {
+	if (fw_arg0 >= CKSEG0 || fw_arg1 < CKSEG0 || fw_arg1 >= CKSEG2) {
 		fw_argc = 0;
 		_fw_argv = NULL;
 	} else {
@@ -31,7 +31,7 @@ void __init fw_init_cmdline(void)
 	}
 
 	/* Validate environment pointer. */
-	if (fw_arg2 < CKSEG0)
+	if (fw_arg2 < CKSEG0 || fw_arg2 >= CKSEG2)
 		_fw_envp = NULL;
 	else
 		_fw_envp = (int *)fw_arg2;
-- 
2.39.2

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Jiaxun Yang]

在2024年8月24日八月 下午3:41，Bjørn Mork写道：
> Boards based on the same SoC family can use different boot loaders.
> These may pass numeric arguments which we erroneously interpret as
> command line or environment pointers. Such errors will cause boot
> to halt at an early stage since commit 056a68cea01e ("mips: allow
> firmware to pass RNG seed to kernel").
>
> One known example of this issue is a HPE switch using a BootWare
> boot loader.  It was found to pass these arguments to the kernel:
>
>   0x00020000 0x00060000 0xfffdffff 0x0000416c
>
> We can avoid hanging by validating that both passed pointers are in
> KSEG1 as expected.

Hi Bjorn,

This is actually breaking 64 bit systems passing fw_args in XKPHYS or KSEG0.

Maybe something like:

static inline bool valid_fw_arg(unsigned long arg)
{
#ifdef CONFIG_64BIT
	if (arg >= XKPHYS && arg < XKSEG)
		return TRUE;
#endif
	return arg >= CKSEG0 && arg < CKSSEG;
}

Will be more robust.

Thanks
- Jiaxun

>
> Cc: stable@vger.kernel.org
> Fixes: 14aecdd41921 ("MIPS: FW: Add environment variable processing.")
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  arch/mips/fw/lib/cmdline.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/fw/lib/cmdline.c b/arch/mips/fw/lib/cmdline.c
> index 892765b742bb..51238c4f9455 100644
> --- a/arch/mips/fw/lib/cmdline.c
> +++ b/arch/mips/fw/lib/cmdline.c
> @@ -22,7 +22,7 @@ void __init fw_init_cmdline(void)
>  	int i;
> 
>  	/* Validate command line parameters. */
> -	if ((fw_arg0 >= CKSEG0) || (fw_arg1 < CKSEG0)) {
> +	if (fw_arg0 >= CKSEG0 || fw_arg1 < CKSEG0 || fw_arg1 >= CKSEG2) {
>  		fw_argc = 0;
>  		_fw_argv = NULL;
>  	} else {
> @@ -31,7 +31,7 @@ void __init fw_init_cmdline(void)
>  	}
> 
>  	/* Validate environment pointer. */
> -	if (fw_arg2 < CKSEG0)
> +	if (fw_arg2 < CKSEG0 || fw_arg2 >= CKSEG2)
>  		_fw_envp = NULL;
>  	else
>  		_fw_envp = (int *)fw_arg2;
> -- 
> 2.39.2

-- 
- Jiaxun

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Maciej W. Rozycki]

On Sun, 25 Aug 2024, Jiaxun Yang wrote:

> Maybe something like:
> 
> static inline bool valid_fw_arg(unsigned long arg)
> {
> #ifdef CONFIG_64BIT
> 	if (arg >= XKPHYS && arg < XKSEG)
> 		return TRUE;
> #endif
> 	return arg >= CKSEG0 && arg < CKSSEG;
> }

 Or rather:

	if (IS_ENABLED(CONFIG_64BIT) && arg >= XKPHYS && arg < XKSEG)
		return TRUE;

as we want to avoid code interspersed with preprocessor conditionals where 
possible (cf. "21) Conditional Compilation" in our coding style document, 
also for the rationale).

  Maciej

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Bjørn Mork]

"Jiaxun Yang" <jiaxun.yang@flygoat.com> writes:
> 在2024年8月24日八月 下午3:41，Bjørn Mork写道：
>> Boards based on the same SoC family can use different boot loaders.
>> These may pass numeric arguments which we erroneously interpret as
>> command line or environment pointers. Such errors will cause boot
>> to halt at an early stage since commit 056a68cea01e ("mips: allow
>> firmware to pass RNG seed to kernel").
>>
>> One known example of this issue is a HPE switch using a BootWare
>> boot loader.  It was found to pass these arguments to the kernel:
>>
>>   0x00020000 0x00060000 0xfffdffff 0x0000416c
>>
>> We can avoid hanging by validating that both passed pointers are in
>> KSEG1 as expected.
>
> Hi Bjorn,
>
> This is actually breaking 64 bit systems passing fw_args in XKPHYS or KSEG0.

Ouch.  Thanks for the feedback.

But if so, then aren't those already broken with the current test
against CKSEG0?  I didn't add that.

IIUC, CKSEGx is the sign extendend version of KSEGx on 64BIT:

#ifdef CONFIG_64BIT

/*
 * Memory segments (64bit kernel mode addresses)
 * The compatibility segments use the full 64-bit sign extended value.  Note
 * the R8000 doesn't have them so don't reference these in generic MIPS code.
 */
#define XKUSEG                  _CONST64_(0x0000000000000000)
#define XKSSEG                  _CONST64_(0x4000000000000000)
#define XKPHYS                  _CONST64_(0x8000000000000000)
#define XKSEG                   _CONST64_(0xc000000000000000)
#define CKSEG0                  _CONST64_(0xffffffff80000000)
#define CKSEG1                  _CONST64_(0xffffffffa0000000)
#define CKSSEG                  _CONST64_(0xffffffffc0000000)
#define CKSEG3                  _CONST64_(0xffffffffe0000000)

#define CKSEG0ADDR(a)           (CPHYSADDR(a) | CKSEG0)
#define CKSEG1ADDR(a)           (CPHYSADDR(a) | CKSEG1)
#define CKSEG2ADDR(a)           (CPHYSADDR(a) | CKSEG2)
#define CKSEG3ADDR(a)           (CPHYSADDR(a) | CKSEG3)


> Maybe something like:
>
> static inline bool valid_fw_arg(unsigned long arg)
> {
> #ifdef CONFIG_64BIT
> 	if (arg >= XKPHYS && arg < XKSEG)
> 		return TRUE;
> #endif
> 	return arg >= CKSEG0 && arg < CKSSEG;
> }
>
> Will be more robust.

Maybe?

But I can't make that match what U-Boot does.  AFAICS,
u-boot/arch/mips/lib/bootm.c doesn't care about 32 or 64 bit, and simply
does:

static void linux_cmdline_init(void)
{
        linux_argc = 1;
        linux_argv = (char **)CKSEG1ADDR(gd->bd->bi_boot_params);
        linux_argv[0] = 0;
        linux_argp = (char *)(linux_argv + LINUX_MAX_ARGS);
}


Then it derives the argument and environment pointers from linux_argv.
Which means that all these pointers end up somewhere between CKSEG1 and
CKSEG2 on both 32 and 64 bit. Or am I missing something?

Tried looking for other MIPS code in U-Boot handling this for 64 bit,
but all I found was arch/mips/mach-octeon/bootoctlinux.c which uses a
completely different protocol.

Sorry if I am asking stupid questions here.  Trying a rather steep
learning curve. Maybe too steep :-)




Bjørn

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Maciej W. Rozycki]

On Sun, 25 Aug 2024, Bjørn Mork wrote:

> But I can't make that match what U-Boot does.  AFAICS,
> u-boot/arch/mips/lib/bootm.c doesn't care about 32 or 64 bit, and simply
> does:

 U-Boot isn't the only firmware used with MIPS systems, and in fact it's 
quite a recent addition; e.g. DEC REX and SGI ARCS firmware goes back at 
least to 1990 and we continue supporting these systems.  There's also CFE, 
PMON, YAMON, etc.

  Maciej

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Bjørn Mork]

"Maciej W. Rozycki" <macro@orcam.me.uk> writes:
> On Sun, 25 Aug 2024, Bjørn Mork wrote:
>
>> But I can't make that match what U-Boot does.  AFAICS,
>> u-boot/arch/mips/lib/bootm.c doesn't care about 32 or 64 bit, and simply
>> does:
>
>  U-Boot isn't the only firmware used with MIPS systems, and in fact it's 
> quite a recent addition; e.g. DEC REX and SGI ARCS firmware goes back at 
> least to 1990 and we continue supporting these systems.  There's also CFE, 
> PMON, YAMON, etc.

I know.  That's what I'm trying to make us support :-)

We can't blindly assume that all those firmwares implement the exact
same command line and environment protocol as U-Boot.  Yamon obviously
uses something similar enough.  But what about the others?

As mentioned, we're currently failing to boot from Bootware because of
the relaxed checking here.  Something needs to be done about that.


Bjørn

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Maciej W. Rozycki]

On Sun, 25 Aug 2024, Bjørn Mork wrote:

> >  U-Boot isn't the only firmware used with MIPS systems, and in fact it's 
> > quite a recent addition; e.g. DEC REX and SGI ARCS firmware goes back at 
> > least to 1990 and we continue supporting these systems.  There's also CFE, 
> > PMON, YAMON, etc.
> 
> I know.  That's what I'm trying to make us support :-)
> 
> We can't blindly assume that all those firmwares implement the exact
> same command line and environment protocol as U-Boot.  Yamon obviously
> uses something similar enough.  But what about the others?

 I can see `fw_init_cmdline' is only used for a handful of newer platforms 
and other ones handle it differently (e.g. arch/mips/dec/prom/cmdline.c).  
Even those that do use the function have a choice to override the default 
handler by setting CONFIG_HAVE_PLAT_FW_INIT_CMDLINE.  Perhaps it's what 
you need to do for your platform too.

 Also lots of MIPS platforms have used `prom_getenv' since long before 
`fw_getenv' and arch/mips/fw/lib/cmdline.c have been added and I have no 
idea why a competing interface was let in back in 2013 with disregard to 
the existing infrastructure.  There is `cfe_getenv' for CFE platforms too. 

 It's clear to me that this mess has to be cleaned up.  Not all kinds of 
firmware permit the setting of arbitrary environment variables (or ones 
that survive a reboot) though.

  Maciej

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Bjørn Mork]

"Maciej W. Rozycki" <macro@orcam.me.uk> writes:

> Even those that do use the function have a choice to override the default 
> handler by setting CONFIG_HAVE_PLAT_FW_INIT_CMDLINE.  Perhaps it's what 
> you need to do for your platform too.

Considered that, but this problem isn't directly platform-related, is
it?  Many of the firmware implementations are multi-platform. And they
support many of the same platforms.

My concrete eample showed up on the "realtek" (rtl838x and rtl93xx SoCs)
platform in OpenWrt, where a large number of boards from assorted
vendors are currently supported. The current implementation works fine
for most of them because they using U-Boot. But it failed in what I
consider the ugliest way possible (no ooops, no error message, just
hanging) on the boards from HPE because they use Bootware.

FWIW, there is also a vendor using BootBase for pretty much the same
hardware.  But that's still unsupported in OpenWrt for various reasons.

So yes, this could be worked around by having a platform specific
fw_init_cmdline().  Or even another out-of-tree OpenWrt specific patch.
But whatever the solution is, it can't drop the U-Boot support since
most of the boards actually use U-Boot.  Which is why I believe it's
much better to solve this problem at the root, for the benefit of
everyone else.

But of course, if necessary it would be possible to build X kernels with
dfferent fw_init_cmdline() implementations to support the same hardware
booted from X different boot firmwares.

>  It's clear to me that this mess has to be cleaned up.  Not all kinds of 
> firmware permit the setting of arbitrary environment variables (or ones 
> that survive a reboot) though.

Yes.  And I believe it can be solved by improving the pointer validation
that's already there.  All we need is to reject argument values passed
by other firmwares.

But it's probably better to create an inline valid_fw_arg(() or similar
function as proposed by Jiaxun, allowing the XKPHYS range too on 64 bit
systems.

If necessary we can improve further by adding an alignment requirement,
which was a proposal that came up in the OpenWrt discussions.

Another option is to connect the validation of all 4 arguments.  There
is for example no reason to interpret the Bootware fw_arg2 value as an
enviroment pointer after we've already rejected fw_arg1 as cmdline.

It's also possible to validate command line argument pointers and
environment variable pointers (except for alignment, I guess?), refusing
anything which ends ut pointing outsde KSEG1 or XKPHYS.

How complicated you want this is all a matter of taste IMHO.  I tried to
make this a simple solution matching the current "forgiving"
implementation.

Another improvement would be a pr_info() dumping the fw_argX values. It
would make it possible to understand why the code is hanging when the
firmware does something unexpected. I don't think pr_debug() helps much
in this particular case.



Bjørn

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[Jiaxun Yang]

在2024年8月25日八月 下午3:44，Bjørn Mork写道：
> "Jiaxun Yang" <jiaxun.yang@flygoat.com> writes:
>> 在2024年8月24日八月 下午3:41，Bjørn Mork写道：
>>> Boards based on the same SoC family can use different boot loaders.
>>> These may pass numeric arguments which we erroneously interpret as
>>> command line or environment pointers. Such errors will cause boot
>>> to halt at an early stage since commit 056a68cea01e ("mips: allow
>>> firmware to pass RNG seed to kernel").
>>>
>>> One known example of this issue is a HPE switch using a BootWare
>>> boot loader.  It was found to pass these arguments to the kernel:
>>>
>>>   0x00020000 0x00060000 0xfffdffff 0x0000416c
>>>
>>> We can avoid hanging by validating that both passed pointers are in
>>> KSEG1 as expected.
>>
>> Hi Bjorn,
>>
>> This is actually breaking 64 bit systems passing fw_args in XKPHYS or KSEG0.
>
> Ouch.  Thanks for the feedback.
>
> But if so, then aren't those already broken with the current test
> against CKSEG0?  I didn't add that.
>

Ah my bad. My impression was it is possible to pass args in XKPHYS with
existing infra (and some PMON bootloader is doing that) but it turns out that
capability was somehow dropped when I was migrating various platforms to
fw_init_cmdline.

Feel free to propose a patch and go with valid_fw_arg, it's always good to have 
robust generic implementation for validating stuff.

In long term we may need to clean up this as Maciej suggested, but IMO going through
jungle of platforms is not a feasible task for casual contributors.

Thanks
-- 
- Jiaxun

Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

[kernel test robot]

Hi Bjørn,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.11-rc5 next-20240828]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Bj-rn-Mork/MIPS-fw-Gracefully-handle-unknown-firmware-protocols/20240826-170643
base:   linus/master
patch link:    https://lore.kernel.org/r/20240824144133.1464835-1-bjorn%40mork.no
patch subject: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols
config: mips-bigsur_defconfig (https://download.01.org/0day-ci/archive/20240829/202408290501.kTATRaBZ-lkp@intel.com/config)
compiler: mips64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240829/202408290501.kTATRaBZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408290501.kTATRaBZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/mips/fw/lib/cmdline.c: In function 'fw_init_cmdline':
>> arch/mips/fw/lib/cmdline.c:25:65: error: 'CKSEG2' undeclared (first use in this function); did you mean 'CKSEG0'?
      25 |         if (fw_arg0 >= CKSEG0 || fw_arg1 < CKSEG0 || fw_arg1 >= CKSEG2) {
         |                                                                 ^~~~~~
         |                                                                 CKSEG0
   arch/mips/fw/lib/cmdline.c:25:65: note: each undeclared identifier is reported only once for each function it appears in


vim +25 arch/mips/fw/lib/cmdline.c

    18	
    19	#ifndef CONFIG_HAVE_PLAT_FW_INIT_CMDLINE
    20	void __init fw_init_cmdline(void)
    21	{
    22		int i;
    23	
    24		/* Validate command line parameters. */
  > 25		if (fw_arg0 >= CKSEG0 || fw_arg1 < CKSEG0 || fw_arg1 >= CKSEG2) {
    26			fw_argc = 0;
    27			_fw_argv = NULL;
    28		} else {
    29			fw_argc = (fw_arg0 & 0x0000ffff);
    30			_fw_argv = (int *)fw_arg1;
    31		}
    32	
    33		/* Validate environment pointer. */
    34		if (fw_arg2 < CKSEG0 || fw_arg2 >= CKSEG2)
    35			_fw_envp = NULL;
    36		else
    37			_fw_envp = (int *)fw_arg2;
    38	
    39		for (i = 1; i < fw_argc; i++) {
    40			strlcat(arcs_cmdline, fw_argv(i), COMMAND_LINE_SIZE);
    41			if (i < (fw_argc - 1))
    42				strlcat(arcs_cmdline, " ", COMMAND_LINE_SIZE);
    43		}
    44	}
    45	#endif
    46	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki