[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.21.2406300615200.43454@angie.orcam.me.uk>
Date: Sun, 30 Jun 2024 07:56:19 +0100 (BST)
From: "Maciej W. Rozycki" <macro@...am.me.uk>
To: Shiji Yang <yangshiji66@...look.com>
cc: linux-mips@...r.kernel.org,
Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Javier Martinez Canillas <javierm@...hat.com>,
Khalid Aziz <khalid@...ehiking.org>, Baoquan He <bhe@...hat.com>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
Serge Semin <fancer.lancer@...il.com>, linux-kernel@...r.kernel.org,
Mieczyslaw Nalewaj <namiltd@...oo.com>
Subject: Re: [PATCH v3] mips: kernel: fix detect_memory_region() function
On Sat, 29 Jun 2024, Shiji Yang wrote:
> 1. Do not use memcmp() on unallocated memory, as the new introduced
> fortify dynamic object size check[1] will return unexpected result.
That seems like a bug in the check to me. Why would `memcmp' referring
to a location within the data section cause an unexpected result, forcing
code to use an equivalent handcoded sequence? This defeats the purpose of
having possibly optimised code in `memcmp' for this.
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index 12a1a4ffb602..4197c7568f49 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -86,21 +86,26 @@ static struct resource bss_resource = { .name = "Kernel bss", };
> unsigned long __kaslr_offset __ro_after_init;
> EXPORT_SYMBOL(__kaslr_offset);
>
> -static void *detect_magic __initdata = detect_memory_region;
> -
> #ifdef CONFIG_MIPS_AUTO_PFN_OFFSET
> unsigned long ARCH_PFN_OFFSET;
> EXPORT_SYMBOL(ARCH_PFN_OFFSET);
> #endif
>
> +#define MIPS_MEM_TEST_PATTERN 0xaa5555aa
> +
> void __init detect_memory_region(phys_addr_t start, phys_addr_t sz_min, phys_addr_t sz_max)
> {
> - void *dm = &detect_magic;
> + u32 detect_magic;
> + volatile u32 *dm = (volatile u32 *)CKSEG1ADDR_OR_64BIT(&detect_magic);
> phys_addr_t size;
>
> for (size = sz_min; size < sz_max; size <<= 1) {
> - if (!memcmp(dm, dm + size, sizeof(detect_magic)))
> - break;
> + *dm = MIPS_MEM_TEST_PATTERN;
> + if (*dm == *(volatile u32 *)((void *)dm + size)) {
> + *dm = ~MIPS_MEM_TEST_PATTERN;
> + if (*dm == *(volatile u32 *)((void *)dm + size))
Can't you just do *(dm + (size >> 2)) and avoid all the horrible casting?
Or maybe even dm[0] == dm[size >> 2]?
> + break;
> + }
> }
Anyway this code makes no sense to me, with or without the change. What
is it exactly supposed to peek at, the location of `detect_magic' plus
some offset?
What about the `start' parameter? What is it for, given that it's not
used in probing, but only for reporting and adding the memory region? Is
it not where probing is supposed to happen in the first place?
I can see how `ath79_detect_mem_size' this mess has come from made some
sense as in 9b75733b7b5e0^:arch/mips/ath79/setup.c -- a bit hackish, but
the size of the probing window set to 1024 bytes combined with comparing
against machine code from `ath79_detect_mem_size' made a false hit highly
unlikely. That sense has been lost since and your change partially fixes
it by making a check against both the straight and the complemented value
of a test pattern. Good.
But still that does not fix the issue with `start'. Given how this code
has been written I'm not even sure if it's suitable for nonzero `start' at
all. Or should the call to `memblock_add' just be adjusted accordingly:
memblock_add(start, size - start);
? This might make sense and if suitable, then it should be documented as
the feature of `detect_memory_region' (as should probably be that it will
round the amount of memory available down to the nearest power of two).
Alternatively the code can start probing at `start', but then it'll have
to be rewritten, because obviously `detect_magic' may not necessarily be
above `start'.
Maciej
Powered by blists - more mailing lists