| 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: <fe79912f-3232-ffba-a191-477c80c703f4@huawei.com>
Date: Mon, 15 May 2023 14:26:31 +0800
From: Gong Ruiqi <gongruiqi1@...wei.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: <linux-mm@...ck.org>, <linux-hardening@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
Hyeonggon Yoo <42.hyeyoo@...il.com>,
<kasan-dev@...glegroups.com>,
Wang Weiyang <wangweiyang2@...wei.com>,
Xiu Jianfeng <xiujianfeng@...wei.com>,
Pedro Falcato <pedro.falcato@...il.com>,
Hyeonggon Yoo <42.hyeyoo@...il.com>,
Vlastimil Babka <vbabka@...e.cz>,
Christoph Lameter <cl@...ux.com>,
David Rientjes <rientjes@...gle.com>,
Roman Gushchin <roman.gushchin@...ux.dev>,
Joonsoo Kim <iamjoonsoo.kim@....com>,
Andrew Morton <akpm@...ux-foundation.org>,
Pekka Enberg <penberg@...nel.org>,
Kees Cook <keescook@...omium.org>,
Paul Moore <paul@...l-moore.com>,
James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>,
"Gustavo A. R. Silva" <gustavoars@...nel.org>
Subject: Re: [PATCH RFC v2] Randomized slab caches for kmalloc()
On 2023/05/11 22:54, Alexander Lobakin wrote:
[...]
>> @@ -777,12 +783,44 @@ EXPORT_SYMBOL(kmalloc_size_roundup);
>> #define KMALLOC_RCL_NAME(sz)
>> #endif
>>
>> +#ifdef CONFIG_RANDOM_KMALLOC_CACHES
>> +#define __KMALLOC_RANDOM_CONCAT(a, b, c) a ## b ## c
>> +#define KMALLOC_RANDOM_NAME(N, sz) __KMALLOC_RANDOM_CONCAT(KMALLOC_RANDOM_, N, _NAME)(sz)
>> +#if CONFIG_RANDOM_KMALLOC_CACHES_BITS >= 1
>> +#define KMALLOC_RANDOM_1_NAME(sz) .name[KMALLOC_RANDOM_START + 0] = "kmalloc-random-01-" #sz,
>> +#define KMALLOC_RANDOM_2_NAME(sz) KMALLOC_RANDOM_1_NAME(sz) .name[KMALLOC_RANDOM_START + 1] = "kmalloc-random-02-" #sz,
>> +#endif
>> +#if CONFIG_RANDOM_KMALLOC_CACHES_BITS >= 2
>> +#define KMALLOC_RANDOM_3_NAME(sz) KMALLOC_RANDOM_2_NAME(sz) .name[KMALLOC_RANDOM_START + 2] = "kmalloc-random-03-" #sz,
>> +#define KMALLOC_RANDOM_4_NAME(sz) KMALLOC_RANDOM_3_NAME(sz) .name[KMALLOC_RANDOM_START + 3] = "kmalloc-random-04-" #sz,
>> +#endif
>> +#if CONFIG_RANDOM_KMALLOC_CACHES_BITS >= 3
>> +#define KMALLOC_RANDOM_5_NAME(sz) KMALLOC_RANDOM_4_NAME(sz) .name[KMALLOC_RANDOM_START + 4] = "kmalloc-random-05-" #sz,
>> +#define KMALLOC_RANDOM_6_NAME(sz) KMALLOC_RANDOM_5_NAME(sz) .name[KMALLOC_RANDOM_START + 5] = "kmalloc-random-06-" #sz,
>> +#define KMALLOC_RANDOM_7_NAME(sz) KMALLOC_RANDOM_6_NAME(sz) .name[KMALLOC_RANDOM_START + 6] = "kmalloc-random-07-" #sz,
>> +#define KMALLOC_RANDOM_8_NAME(sz) KMALLOC_RANDOM_7_NAME(sz) .name[KMALLOC_RANDOM_START + 7] = "kmalloc-random-08-" #sz,
>> +#endif
>> +#if CONFIG_RANDOM_KMALLOC_CACHES_BITS >= 4
>> +#define KMALLOC_RANDOM_9_NAME(sz) KMALLOC_RANDOM_8_NAME(sz) .name[KMALLOC_RANDOM_START + 8] = "kmalloc-random-09-" #sz,
>> +#define KMALLOC_RANDOM_10_NAME(sz) KMALLOC_RANDOM_9_NAME(sz) .name[KMALLOC_RANDOM_START + 9] = "kmalloc-random-10-" #sz,
>> +#define KMALLOC_RANDOM_11_NAME(sz) KMALLOC_RANDOM_10_NAME(sz) .name[KMALLOC_RANDOM_START + 10] = "kmalloc-random-11-" #sz,
>> +#define KMALLOC_RANDOM_12_NAME(sz) KMALLOC_RANDOM_11_NAME(sz) .name[KMALLOC_RANDOM_START + 11] = "kmalloc-random-12-" #sz,
>> +#define KMALLOC_RANDOM_13_NAME(sz) KMALLOC_RANDOM_12_NAME(sz) .name[KMALLOC_RANDOM_START + 12] = "kmalloc-random-13-" #sz,
>> +#define KMALLOC_RANDOM_14_NAME(sz) KMALLOC_RANDOM_13_NAME(sz) .name[KMALLOC_RANDOM_START + 13] = "kmalloc-random-14-" #sz,
>> +#define KMALLOC_RANDOM_15_NAME(sz) KMALLOC_RANDOM_14_NAME(sz) .name[KMALLOC_RANDOM_START + 14] = "kmalloc-random-15-" #sz,
>> +#define KMALLOC_RANDOM_16_NAME(sz) KMALLOC_RANDOM_15_NAME(sz) .name[KMALLOC_RANDOM_START + 15] = "kmalloc-random-16-" #sz,
>
> This all can be compressed. Only two things are variables here, so
>
> #define KMALLOC_RANDOM_N_NAME(cur, prev, sz) \
> KMALLOC_RANDOM_##prev##_NAME(sz), \
> .name[KMALLOC_RANDOM_START + prev] = \
> "kmalloc-random-##cur##-" #sz
>
> #define KMALLOC_RANDOM_16_NAME(sz) KMALLOC_RANDOM_N_NAME(16, 15, sz)
>
I tried this way of implementation but it didn't work: it did not
propagate from 16 to 1, but stopped in the middle. I think it's because
the macro is somehow (indirectly) self-referential and the preprocessor
won't expand it. Check this for more info:
https://gcc.gnu.org/onlinedocs/cpp/Self-Referential-Macros.html
> Also I'd rather not put commas ',' at the end of each macro, they're
> usually put outside where the macro is used.
It seems here we have to put commas at the end. Not only it's to align
with how KMALLOC_{RCL,CGROUP,DMA}_NAME are implemented, but also
otherwise the expansion of INIT_KMALLOC_INFO would in some cases be like:
{
.name[KMALLOC_NORMAL] = "kmalloc-" #__short_size,
, // an empty entry with a comma
}
which would cause compilation error in kmalloc_info[]'s initialization.
>> +#endif
>> +#else // CONFIG_RANDOM_KMALLOC_CACHES
>> +#define KMALLOC_RANDOM_NAME(N, sz)
>> +#endif
>> +
>> #define INIT_KMALLOC_INFO(__size, __short_size) \
>> { \
>> .name[KMALLOC_NORMAL] = "kmalloc-" #__short_size, \
>> KMALLOC_RCL_NAME(__short_size) \
>> KMALLOC_CGROUP_NAME(__short_size) \
>> KMALLOC_DMA_NAME(__short_size) \
>> + KMALLOC_RANDOM_NAME(CONFIG_RANDOM_KMALLOC_CACHES_NR, __short_size) \
>
> Can't those names be __initconst and here you'd just do one loop from 1
> to KMALLOC_CACHES_NR, which would assign names? I'm not sure compilers
> will expand that one to a compile-time constant and assigning 69
> different string pointers per one kmalloc size is a bit of a waste to me.
I'm not sure if I understand the question correctly, but I believe these
names have been __initconst since kmalloc_info[] is already marked with
it. Please let me know if it doesn't answer your question.
>> .size = __size, \
>> }
>>
>> @@ -878,6 +916,11 @@ new_kmalloc_cache(int idx, enum kmalloc_cache_type type, slab_flags_t flags)
>> flags |= SLAB_CACHE_DMA;
>> }
>>
>> +#ifdef CONFIG_RANDOM_KMALLOC_CACHES
>> + if (type >= KMALLOC_RANDOM_START && type <= KMALLOC_RANDOM_END)
>> + flags |= SLAB_RANDOMSLAB;
>> +#endif
>> +
>> kmalloc_caches[type][idx] = create_kmalloc_cache(
>> kmalloc_info[idx].name[type],
>> kmalloc_info[idx].size, flags, 0,
>> @@ -904,7 +947,7 @@ void __init create_kmalloc_caches(slab_flags_t flags)
>> /*
>> * Including KMALLOC_CGROUP if CONFIG_MEMCG_KMEM defined
>> */
>> - for (type = KMALLOC_NORMAL; type < NR_KMALLOC_TYPES; type++) {
>> + for (type = KMALLOC_RANDOM_START; type < NR_KMALLOC_TYPES; type++) {
>
> Can't we just define something like __KMALLOC_TYPE_START at the
> beginning of the enum to not search for all such places each time
> something new is added?
Yeah I'm okay with this. Before I apply this change I would like to know
more opinions (especially from the maintainers) about it.
>
>> for (i = KMALLOC_SHIFT_LOW; i <= KMALLOC_SHIFT_HIGH; i++) {
>> if (!kmalloc_caches[type][i])
>> new_kmalloc_cache(i, type, flags);
>> @@ -922,6 +965,9 @@ void __init create_kmalloc_caches(slab_flags_t flags)
>> new_kmalloc_cache(2, type, flags);
>> }
>> }
>> +#ifdef CONFIG_RANDOM_KMALLOC_CACHES
>> + random_kmalloc_seed = get_random_u64();
>> +#endif
>>
>> /* Kmalloc array is now usable */
>> slab_state = UP;
>> @@ -957,7 +1003,7 @@ void *__do_kmalloc_node(size_t size, gfp_t flags, int node, unsigned long caller
>> return ret;
>> }
>>
>> - s = kmalloc_slab(size, flags);
>> + s = kmalloc_slab(size, flags, caller);
>>
>> if (unlikely(ZERO_OR_NULL_PTR(s)))
>> return s;
>
> Thanks,
> Olek
Powered by blists - more mailing lists