linux-kernel - Re: [PATCH 06/11] change memory_is_poisoned

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: <CACT4Y+Ym3kq5RZ-4F=f97bvT2pNpzDf0kerf6tebzLOY_crR8Q@mail.gmail.com>
Date:   Thu, 12 Oct 2017 09:16:39 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     Abbott Liu <liuwenliang@...wei.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        afzal.mohd.ma@...il.com, f.fainelli@...il.com,
        Laura Abbott <labbott@...hat.com>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
        Michal Hocko <mhocko@...e.com>, cdall@...aro.org,
        marc.zyngier@....com, Catalin Marinas <catalin.marinas@....com>,
        Matthew Wilcox <mawilcox@...rosoft.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Thomas Garnier <thgarnie@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>,
        Vladimir Murzin <vladimir.murzin@....com>, tixy@...aro.org,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        robin.murphy@....com, Ingo Molnar <mingo@...nel.org>,
        grygorii.strashko@...aro.org,
        Alexander Potapenko <glider@...gle.com>, opendmb@...il.com,
        linux-arm-kernel@...ts.infradead.org,
        LKML <linux-kernel@...r.kernel.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>, jiazhenghua@...wei.com,
        dylix.dailei@...wei.com, zengweilin@...wei.com,
        heshaoliang@...wei.com
Subject: Re: [PATCH 06/11] change memory_is_poisoned_16 for aligned error

On Thu, Oct 12, 2017 at 1:23 AM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Wed, 11 Oct 2017 16:22:22 +0800 Abbott Liu <liuwenliang@...wei.com> wrote:
>
>>  Because arm instruction set don't support access the address which is
>>  not aligned, so must change memory_is_poisoned_16 for arm.
>>
>> ...
>>
>> --- a/mm/kasan/kasan.c
>> +++ b/mm/kasan/kasan.c
>> @@ -149,6 +149,25 @@ static __always_inline bool memory_is_poisoned_2_4_8(unsigned long addr,
>>       return memory_is_poisoned_1(addr + size - 1);
>>  }
>>
>> +#ifdef CONFIG_ARM
>> +static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> +{
>> +     u8 *shadow_addr = (u8 *)kasan_mem_to_shadow((void *)addr);
>> +
>> +     if (unlikely(shadow_addr[0] || shadow_addr[1])) return true;
>
> Coding-style is messed up.  Please use scripts/checkpatch.pl.
>
>> +     else {
>> +             /*
>> +              * If two shadow bytes covers 16-byte access, we don't
>> +              * need to do anything more. Otherwise, test the last
>> +              * shadow byte.
>> +              */
>> +             if (likely(IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>> +                     return false;
>> +             return memory_is_poisoned_1(addr + 15);
>> +     }
>> +}
>> +
>> +#else
>>  static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>  {
>>       u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>> @@ -159,6 +178,7 @@ static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>
>>       return *shadow_addr;
>>  }
>> +#endif
>
> - 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;
}