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

["Bjørn Mork" <bjorn@mork.no>]

"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