| Message ID | TYCP286MB0895F65439037ED134FEA7DDBCD12@TYCP286MB0895.JPNP286.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] mips: kernel: fix detect_memory_region() function | expand |
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
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)) + break; + } } pr_debug("Memory: %lluMB of RAM detected at 0x%llx (min: %lluMB, max: %lluMB)\n",
1. Do not use memcmp() on unallocated memory, as the new introduced fortify dynamic object size check[1] will return unexpected result. 2. Use a fixed pattern instead of a random function pointer as the magic value. "0xaa5555aa" has a large information entropy and is widely used in memory testing. 3. Flip the magic value and double check it to ensure memory overlap. Tested on ralink/mt7620 and ath79/ar9344 [1] commit 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available") Signed-off-by: Shiji Yang <yangshiji66@outlook.com> Tested-by: Mieczyslaw Nalewaj <namiltd@yahoo.com> --- changes: v2 -> v3: * Using CKSEG1ADDR_OR_64BIT() instead of excluding 64bit system. * Using volatile pointer to get and compare u32 data. v1: https://lore.kernel.org/all/TYAP286MB0315E609C476B86E22700626BC292@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/ v2: https://lore.kernel.org/all/TYCP286MB089598ABD1E2F66003D71EB8BCD52@TYCP286MB0895.JPNP286.PROD.OUTLOOK.COM/ arch/mips/kernel/setup.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)