| 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: <ynl7b325d5jo52n7cpy64v6bvqhzlbkphqsbs3jrgtji4v4yoz@cjpytwlwc6kt>
Date: Wed, 9 Apr 2025 18:32:36 +0200
From: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
To: Dave Hansen <dave.hansen@...el.com>
CC: <hpa@...or.com>, <hch@...radead.org>, <nick.desaulniers+lkml@...il.com>,
<kuan-ying.lee@...onical.com>, <masahiroy@...nel.org>,
<samuel.holland@...ive.com>, <mingo@...hat.com>, <corbet@....net>,
<ryabinin.a.a@...il.com>, <guoweikang.kernel@...il.com>,
<jpoimboe@...nel.org>, <ardb@...nel.org>, <vincenzo.frascino@....com>,
<glider@...gle.com>, <kirill.shutemov@...ux.intel.com>, <apopple@...dia.com>,
<samitolvanen@...gle.com>, <kaleshsingh@...gle.com>, <jgross@...e.com>,
<andreyknvl@...il.com>, <scott@...amperecomputing.com>,
<tony.luck@...el.com>, <dvyukov@...gle.com>, <pasha.tatashin@...een.com>,
<ziy@...dia.com>, <broonie@...nel.org>, <gatlin.newhouse@...il.com>,
<jackmanb@...gle.com>, <wangkefeng.wang@...wei.com>,
<thiago.bauermann@...aro.org>, <tglx@...utronix.de>, <kees@...nel.org>,
<akpm@...ux-foundation.org>, <jason.andryuk@....com>, <snovitoll@...il.com>,
<xin@...or.com>, <jan.kiszka@...mens.com>, <bp@...en8.de>, <rppt@...nel.org>,
<peterz@...radead.org>, <pankaj.gupta@....com>, <thuth@...hat.com>,
<andriy.shevchenko@...ux.intel.com>, <joel.granados@...nel.org>,
<kbingham@...nel.org>, <nicolas@...sle.eu>, <mark.rutland@....com>,
<surenb@...gle.com>, <catalin.marinas@....com>, <morbo@...gle.com>,
<justinstitt@...gle.com>, <ubizjak@...il.com>, <jhubbard@...dia.com>,
<urezki@...il.com>, <dave.hansen@...ux.intel.com>, <bhe@...hat.com>,
<luto@...nel.org>, <baohua@...nel.org>, <nathan@...nel.org>,
<will@...nel.org>, <brgerst@...il.com>, <llvm@...ts.linux.dev>,
<linux-mm@...ck.org>, <linux-doc@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kbuild@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <kasan-dev@...glegroups.com>,
<x86@...nel.org>
Subject: Re: [PATCH v3 13/14] mm: Unpoison pcpu chunks with base address tag
On 2025-04-04 at 11:08:12 -0700, Dave Hansen wrote:
>On 4/4/25 06:14, Maciej Wieczor-Retman wrote:
>> The problem presented here is related to NUMA systems and tag-based
>> KASAN mode. Getting to it can be explained in the following points:
>>
>> 1. A new chunk is created with pcpu_create_chunk() and
>> vm_structs are allocated. On systems with one NUMA node only
>> one is allocated, but with more NUMA nodes at least a second
>> one will be allocated too.
>>
>> 2. chunk->base_addr is assigned the modified value of
>> vms[0]->addr and thus inherits the tag of this allocated
>> structure.
>>
>> 3. In pcpu_alloc() for each possible cpu pcpu_chunk_addr() is
>> executed which calculates per cpu pointers that correspond to
>> the vms structure addresses. The calculations are based on
>> adding an offset from a table to chunk->base_addr.
>>
>> Here the problem presents itself since for addresses based on vms[1] and
>> up, the tag will be different than the ones based on vms[0] (base_addr).
>> The tag mismatch happens and an error is reported.
>>
>> Unpoison all the vms[]->addr with the same tag to resolve the mismatch.
>
>I think there's a bit too much superfluous information in there. For
>instance, it's not important to talk about how or why there can be more
>than one chunk, just say there _can_ be more than one.
>
> 1. There can be more than one chunk
> 2. The chunks are virtually contiguous
> 3. Since they are virtually contiguous, the chunks are all
> addressed from a single base address
> 4. The base address has a tag
> 5. The base address points at the first chunk and thus inherits
> the tag of the first chunk
> 6. The subsequent chunks will be accessed with the tag from the
> first chunk
> 7. Thus, the subsequent chunks need to have their tag set to
> match that of the first chunk.
>
>Right?
They don't seem to be virtuall contiguous. At least from testing on a live
system, QEMU and Simics I never saw any be contiguous. And I double checked
today too. But your version is nice, I'll just drop 2 and 3 and I think it still
will make sense, right?
>
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index 54481f8c30c5..bd033b2ba383 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -613,6 +613,13 @@ static __always_inline void kasan_poison_vmalloc(const void *start,
>> __kasan_poison_vmalloc(start, size);
>> }
>>
>> +void __kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms);
>> +static __always_inline void kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms)
>> +{
>> + if (kasan_enabled())
>> + __kasan_unpoison_vmap_areas(vms, nr_vms);
>> +}
>> +
>> #else /* CONFIG_KASAN_VMALLOC */
>>
>> static inline void kasan_populate_early_vm_area_shadow(void *start,
>> @@ -637,6 +644,9 @@ static inline void *kasan_unpoison_vmalloc(const void *start,
>> static inline void kasan_poison_vmalloc(const void *start, unsigned long size)
>> { }
>>
>> +static inline void kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms)
>> +{ }
>> +
>> #endif /* CONFIG_KASAN_VMALLOC */
>>
>> #if (defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)) && \
>> diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
>> index 88d1c9dcb507..9496f256bc0f 100644
>> --- a/mm/kasan/shadow.c
>> +++ b/mm/kasan/shadow.c
>> @@ -582,6 +582,17 @@ void __kasan_poison_vmalloc(const void *start, unsigned long size)
>> kasan_poison(start, size, KASAN_VMALLOC_INVALID, false);
>> }
>>
>> +void __kasan_unpoison_vmap_areas(struct vm_struct **vms, int nr_vms)
>> +{
>> + int area;
>> +
>> + for (area = 0 ; area < nr_vms ; area++) {
>> + kasan_poison(vms[area]->addr, vms[area]->size,
>> + arch_kasan_get_tag(vms[0]->addr), false);
>> + arch_kasan_set_tag(vms[area]->addr, arch_kasan_get_tag(vms[0]->addr));
>> + }
>> +}
>
>-ENOCOMMENTS
Right, I'll add a description why that's needed.
>
>> #else /* CONFIG_KASAN_VMALLOC */
>>
>> int kasan_alloc_module_shadow(void *addr, size_t size, gfp_t gfp_mask)
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index 61981ee1c9d2..fbd56bf8aeb2 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -4783,8 +4783,7 @@ struct vm_struct **pcpu_get_vm_areas(const unsigned long *offsets,
>> * non-VM_ALLOC mappings, see __kasan_unpoison_vmalloc().
>> */
>> for (area = 0; area < nr_vms; area++)
>> - vms[area]->addr = kasan_unpoison_vmalloc(vms[area]->addr,
>> - vms[area]->size, KASAN_VMALLOC_PROT_NORMAL);
>> + kasan_unpoison_vmap_areas(vms, nr_vms);
>>
>> kfree(vas);
>> return vms;
>
>So, the right way to do this is refactor, first, then add your changes
>after. This really wants to be two patches.
Sure, I'll try splitting it.
--
Kind regards
Maciej Wieczór-Retman
Powered by blists - more mailing lists