On Nov 23, 2017 20:30 Russell King - ARM Linux [mailto:[email protected]]
wrote:
>On Thu, Oct 12, 2017 at 11:27:40AM +0000, Liuwenliang (Lamb) wrote:
>> >> - I don't understand why this is necessary. memory_is_poisoned_16()
>> >> already handles unaligned addresses?
>> >>
>> >> - If it's needed on ARM then presumably it will be needed on other
>> >> architectures, so CONFIG_ARM is insufficiently general.
>> >>
>> >> - If the present memory_is_poisoned_16() indeed doesn't work on ARM,
>> >> it would be better to generalize/fix it in some fashion rather than
>> >> creating a new variant of the function.
>>
>>
>> >Yes, I think it will be better to fix the current function rather then
>> >have 2 slightly different copies with ifdef's.
>> >Will something along these lines work for arm? 16-byte accesses are
>> >not too common, so it should not be a performance problem. And
>> >probably modern compilers can turn 2 1-byte checks into a 2-byte check
>> >where safe (x86).
>>
>> >static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> >{
>> > u8 *shadow_addr = (u8 *)kasan_mem_to_shadow((void *)addr);
>> >
>> > if (shadow_addr[0] || shadow_addr[1])
>> > return true;
>> > /* Unaligned 16-bytes access maps into 3 shadow bytes. */
>> > if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>> > return memory_is_poisoned_1(addr + 15);
>> > return false;
>> >}
>>
>> Thanks for Andrew Morton and Dmitry Vyukov's review.
>> If the parameter addr=0xc0000008, now in function:
>> static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> {
>> --- //shadow_addr = (u16 *)(KASAN_OFFSET+0x18000001(=0xc0000008>>3)) is
>> not
>> --- // unsigned by 2 bytes.
>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>
>> /* Unaligned 16-bytes access maps into 3 shadow bytes. */
>> if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>> return *shadow_addr || memory_is_poisoned_1(addr + 15);
>> ---- //here is going to be error on arm, specially when kernel has not
>> finished yet.
>> ---- //Because the unsigned accessing cause DataAbort Exception which
>> is not
>> ---- //initialized when kernel is starting.
>> return *shadow_addr;
>> }
>>
>> I also think it is better to fix this problem.
>What about using get_unaligned() ?
Thanks for your review.
I think it is good idea to use get_unaligned. But ARMv7 support CONFIG_
HAVE_EFFICIENT_UNALIGNED_ACCESS
(arch/arm/Kconfig : select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 ||
CPU_V6K || CPU_V7) && MMU).
So on ARMv7, the code:
u16 *shadow_addr = get_unaligned((u16 *)kasan_mem_to_shadow((void *)addr));
equals the code:000
u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
On ARMv7, if SCRLR.A is 0, unaligned access is OK. Here is the description
comes from ARM(r) Architecture Reference
Manual ARMv7-A and ARMv7-R edition :
A3.2.1 Unaligned data access
An ARMv7 implementation must support unaligned data accesses by some load and
store instructions, as
Table A3-1 shows. Software can set the SCTLR.A bit to control whether a
misaligned access by one of these
instructions causes an Alignment fault Data abort exception.
Table A3-1 Alignment requirements of load/store instructions
Instructions Alignment check SCTLR.A is 0
SCTLR.A is 1
LDRB, LDREXB, LDRBT,
LDRSB, LDRSBT, STRB, None - -
STREXB, STRBT, SWPB,
TBB
LDRH, LDRHT, LDRSH,
LDRSHT, STRH, STRHT, Halfword Unaligned access
Alignment fault
TBH
LDREXH, STREXH Halfword Alignment fault
Alignment fault
LDR, LDRT, STR, STRT
PUSH, encodings T3 and A2 only Word Unaligned access
Alignment fault
POP, encodings T3 and A2 only
LDREX, STREX Word Alignment fault
Alignment fault
LDREXD, STREXD Doubleword Alignment fault
Alignment fault
All forms of LDM and STM,
LDRD, RFE, SRS, STRD, SWP
PUSH, except for encodings
T3 and A2 Word Alignment fault
Alignment fault
POP, except for encodings
T3 and A2
LDC, LDC2, STC, STC2 Word Alignment fault
Alignment fault
VLDM, VLDR, VPOP,
VPUSH, VSTM, VSTR Word Alignment fault
Alignment fault
VLD1, VLD2, VLD3, VLD4,
VST1, VST2, VST3, VST4, Element size Unaligned access
Alignment fault
all with standard alignmenta
VLD1, VLD2, VLD3, VLD4,
VST1, VST2, VST3, VST4, As specified by@<align> Alignment fault
Alignment fault
all with @<align> specifieda
On ARMv7, the following code can guarantee that if SCRLR.A is 0:
__enable_mmu:
#if defined(CONFIG_ALIGNMENT_TRAP) && __LINUX_ARM_ARCH__ < 6
orr r0, r0, #CR_A
#else
bic r0, r0, #CR_A //clear CR_A
#endif
#ifdef CONFIG_CPU_DCACHE_DISABLE
bic r0, r0, #CR_C
#endif
#ifdef CONFIG_CPU_BPREDICT_DISABLE
bic r0, r0, #CR_Z
#endif
#ifdef CONFIG_CPU_ICACHE_DISABLE
bic r0, r0, #CR_I
#endif
#ifdef CONFIG_ARM_LPAE
mcrr p15, 0, r4, r5, c2 @ load TTBR0
#else
mov r5, #DACR_INIT
mcr p15, 0, r5, c3, c0, 0 @ load domain access register
mcr p15, 0, r4, c2, c0, 0 @ load page table pointer
#endif
b __turn_mmu_on
ENDPROC(__enable_mmu)
/*
* Enable the MMU. This completely changes the structure of the visible
* memory space. You will not be able to trace execution through this.
* If you have an enquiry about this, *please* check the linux-arm-kernel
* mailing list archives BEFORE sending another post to the list.
*
* r0 = cp#15 control register
* r1 = machine ID
* r2 = atags or dtb pointer
* r9 = processor ID
* r13 = *virtual* address to jump to upon completion
*
* other registers depend on the function called upon completion
*/
.align 5
.pushsection .idmap.text, "ax"
ENTRY(__turn_mmu_on)
mov r0, r0
instr_sync
mcr p15, 0, r0, c1, c0, 0 @ write control reg //here
set SCTLR=r0
mrc p15, 0, r3, c0, c0, 0 @ read id reg
instr_sync
mov r3, r3
mov r3, r13
ret r3
__turn_mmu_on_end:
ENDPROC(__turn_mmu_on)
So the following code is OK:
static __always_inline bool memory_is_poisoned_16(unsigned long addr)
{
- u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
+ u16 *shadow_addr = get_unaligned( (u16 *)kasan_mem_to_shadow((void
*)addr));
/* Unaligned 16-bytes access maps into 3 shadow bytes. */
if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
return *shadow_addr || memory_is_poisoned_1(addr + 15);
return *shadow_addr;
}
A very good suggestion, Thanks.
