| 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
| ||
|
Message-ID: <CAAeHK+xMjDwZkKDn_0aVWERkjv6B-hFKjn0coGo4LbcPBds4Ew@mail.gmail.com>
Date: Wed, 19 Sep 2018 13:54:23 +0200
From: Andrey Konovalov <andreyknvl@...gle.com>
To: Dmitry Vyukov <dvyukov@...gle.com>
Cc: Andrey Ryabinin <aryabinin@...tuozzo.com>,
Alexander Potapenko <glider@...gle.com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will.deacon@....com>,
Christoph Lameter <cl@...ux.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Mark Rutland <mark.rutland@....com>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Marc Zyngier <marc.zyngier@....com>,
Dave Martin <dave.martin@....com>,
Ard Biesheuvel <ard.biesheuvel@...aro.org>,
"Eric W . Biederman" <ebiederm@...ssion.com>,
Ingo Molnar <mingo@...nel.org>,
Paul Lawrence <paullawrence@...gle.com>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Arnd Bergmann <arnd@...db.de>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Kate Stewart <kstewart@...uxfoundation.org>,
Mike Rapoport <rppt@...ux.vnet.ibm.com>,
kasan-dev <kasan-dev@...glegroups.com>,
"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
linux-sparse@...r.kernel.org, Linux-MM <linux-mm@...ck.org>,
"open list:KERNEL BUILD + fi..." <linux-kbuild@...r.kernel.org>,
Kostya Serebryany <kcc@...gle.com>,
Evgeniy Stepanov <eugenis@...gle.com>,
Lee Smith <Lee.Smith@....com>,
Ramana Radhakrishnan <Ramana.Radhakrishnan@....com>,
Jacob Bramley <Jacob.Bramley@....com>,
Ruben Ayrapetyan <Ruben.Ayrapetyan@....com>,
Jann Horn <jannh@...gle.com>,
Mark Brand <markbrand@...gle.com>,
Chintan Pandya <cpandya@...eaurora.org>,
Vishwath Mohan <vishwath@...gle.com>
Subject: Re: [PATCH v6 14/18] khwasan: add hooks implementation
On Wed, Sep 12, 2018 at 8:30 PM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
> On Wed, Aug 29, 2018 at 1:35 PM, Andrey Konovalov <andreyknvl@...gle.com> wrote:
>> void kasan_unpoison_shadow(const void *address, size_t size)
>> {
>> - kasan_poison_shadow(address, size, 0);
>> + u8 tag = get_tag(address);
>> +
>> + /* Perform shadow offset calculation based on untagged address */
>
> The comment is not super-useful. It would be more useful to say why we
> need to do this.
> Most callers explicitly untag pointer passed to this function, for
> some it's unclear if the pointer contains tag or not.
> For example, __hwasan_tag_memory -- what does it accept? Tagged or untagged?
There are some callers that pass tagged pointers to this functions,
e.g. ksize or kasan_unpoison_object_data. I'll expand the comment.
>
>
>> + address = reset_tag(address);
>> +
>> + kasan_poison_shadow(address, size, tag);
>>
>> if (size & KASAN_SHADOW_MASK) {
>> u8 *shadow = (u8 *)kasan_mem_to_shadow(address + size);
>> - *shadow = size & KASAN_SHADOW_MASK;
>> +
>> + if (IS_ENABLED(CONFIG_KASAN_HW))
>> + *shadow = tag;
>> + else
>> + *shadow = size & KASAN_SHADOW_MASK;
>> }
>> }
>
>
> It seems that this function is just different for kasan and khwasan.
> Currently for kasan we have:
>
> kasan_poison_shadow(address, size, tag);
> if (size & KASAN_SHADOW_MASK) {
> u8 *shadow = (u8 *)kasan_mem_to_shadow(address + size);
> *shadow = size & KASAN_SHADOW_MASK;
> }
>
> But what we want to say for khwasan is:
>
> kasan_poison_shadow(address, round_up(size, KASAN_SHADOW_SCALE_SIZE),
> get_tag(address));
>
> Not sure if we want to keep a common implementation or just have
> separate implementations...
As per offline discussion leaving as is.
>> void kasan_free_pages(struct page *page, unsigned int order)
>> @@ -235,6 +248,7 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>> slab_flags_t *flags)
>> {
>> unsigned int orig_size = *size;
>> + unsigned int redzone_size = 0;
>
> This variable seems to be always initialized below. We don't general
> initialize local variables in this case.
Will fix in v7.
>> void check_memory_region(unsigned long addr, size_t size, bool write,
>> unsigned long ret_ip)
>> {
>> + u8 tag;
>> + u8 *shadow_first, *shadow_last, *shadow;
>> + void *untagged_addr;
>> +
>> + tag = get_tag((const void *)addr);
>> +
>> + /* Ignore accesses for pointers tagged with 0xff (native kernel
>
> /* on a separate line
Will fix in v7.
>
>> + * pointer tag) to suppress false positives caused by kmap.
>> + *
>> + * Some kernel code was written to account for archs that don't keep
>> + * high memory mapped all the time, but rather map and unmap particular
>> + * pages when needed. Instead of storing a pointer to the kernel memory,
>> + * this code saves the address of the page structure and offset within
>> + * that page for later use. Those pages are then mapped and unmapped
>> + * with kmap/kunmap when necessary and virt_to_page is used to get the
>> + * virtual address of the page. For arm64 (that keeps the high memory
>> + * mapped all the time), kmap is turned into a page_address call.
>> +
>> + * The issue is that with use of the page_address + virt_to_page
>> + * sequence the top byte value of the original pointer gets lost (gets
>> + * set to KHWASAN_TAG_KERNEL (0xFF).
>
> Missed closing bracket.
Will fix in v7.
Powered by blists - more mailing lists