linux-kernel - Re: [PATCH 3/3] ARM: Add support for CONFIG_DEBUG

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: <22eb9bb0-f1dc-7923-b7f5-e278c19b374c@gmail.com>
Date:   Tue, 6 Dec 2016 18:24:35 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Laura Abbott <labbott@...hat.com>,
        linux-arm-kernel@...ts.infradead.org
Cc:     linux@...linux.org.uk, nicolas.pitre@...aro.org, panand@...hat.com,
        chris.brandt@...esas.com, arnd@...db.de, jonathan.austin@....com,
        pawel.moll@....com, vladimir.murzin@....com, mark.rutland@....com,
        ard.biesheuvel@...aro.org, keescook@...omium.org,
        matt@...eblueprint.co.uk, kirill.shutemov@...ux.intel.com,
        ben@...adent.org.uk, js07.lee@...sung.com, stefan@...er.ch,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] ARM: Add support for CONFIG_DEBUG_VIRTUAL

On 12/06/2016 06:00 PM, Laura Abbott wrote:
>> @@ -261,6 +261,16 @@ static inline unsigned long __phys_to_virt(phys_addr_t x)
>>  	((((unsigned long)(kaddr) - PAGE_OFFSET) >> PAGE_SHIFT) + \
>>  	 PHYS_PFN_OFFSET)
>>  
>> +#define __pa_symbol_nodebug(x)	((x) - (unsigned long)KERNEL_START)
> 
> On arm64 the kernel image lives in a separate linear offset. arm doesn't
> do anything like that so __phys_addr_symbol should just be the regular
> __virt_to_phys

Yep, which is what I have queued locally now too, thanks!


>> +static inline bool __virt_addr_valid(unsigned long x)
>> +{
>> +	if (x < PAGE_OFFSET)
>> +		return false;
>> +	if (arm_lowmem_limit && is_vmalloc_or_module_addr((void *)x))
>> +		return false;
>> +	if (x >= FIXADDR_START && x < FIXADDR_END)
>> +		return false;
>> +	return true;
>> +}
> 
> I'd rather see this return true for only the linear range and
> reject everything else. asm/memory.h already has
> 
> #define virt_addr_valid(kaddr)  (((unsigned long)(kaddr) >= PAGE_OFFSET && (unsigned long)(kaddr) < (unsigned long)high_memory) \
>                                         && pfn_valid(virt_to_pfn(kaddr)))
> 
> So we can make the check x >= PAGE_OFFSET && x < high_memory

OK that's simpler indeed. I did the check this way because we have early
callers of __pa() from drivers/of/fdt.c, in particular MIN_MEMBLOCK_ADDR
there, and we also have pcpu_dfl_fc_alloc which uses DMA_MAX_ADDR (which
is 0xffff_ffff on my platform).

>> +static inline bool __phys_addr_valid(unsigned long x)
>> +{
>> +	/* This is bounds checking against the kernel image only.
>> +	 * __pa_symbol should only be used on kernel symbol addresses.
>> +	 */
>> +	if (x < (unsigned long)KERNEL_START ||
>> +	    x > (unsigned long)KERNEL_END)
>> +		return false;
>> +
>> +	return true;
>> +}
> 
> This is a confusing name for this function, it's not checking if
> a physical address is valid, it's checking if a virtual address
> corresponding to a kernel symbol is valid.

I have removed it and just moved the check within VIRTUAL_BUG_ON().

Thanks!
-- 
Florian