Re: [PATCH v3] mips: kernel: fix detect_memory_region() function

[Maciej W. Rozycki]

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