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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87o75gy85b.fsf@miraculix.mork.no>
Date: Sun, 25 Aug 2024 16:44:16 +0200
From: Bjørn Mork <bjorn@...k.no>
To: Jiaxun Yang <jiaxun.yang@...goat.com>
Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        "Steven J . Hill" <Steven.Hill@...tec.com>,
        "linux-mips@...r.kernel.org" <linux-mips@...r.kernel.org>,
        linux-kernel@...r.kernel.org,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH] MIPS: fw: Gracefully handle unknown firmware protocols

"Jiaxun Yang" <jiaxun.yang@...goat.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