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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 16 Mar 2021 09:01:12 +0200 From: Topi Miettinen <toiwoton@...il.com> To: Uladzislau Rezki <urezki@...il.com> Cc: linux-hardening@...r.kernel.org, akpm@...ux-foundation.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, Andy Lutomirski <luto@...nel.org>, Jann Horn <jannh@...gle.com>, Kees Cook <keescook@...omium.org>, Linux API <linux-api@...r.kernel.org>, Matthew Wilcox <willy@...radead.org>, Mike Rapoport <rppt@...nel.org> Subject: Re: [PATCH v4] mm/vmalloc: randomize vmalloc() allocations On 15.3.2021 20.02, Uladzislau Rezki wrote: > On Mon, Mar 15, 2021 at 06:23:37PM +0200, Topi Miettinen wrote: >> On 15.3.2021 17.35, Uladzislau Rezki wrote: >>>> On 14.3.2021 19.23, Uladzislau Rezki wrote: >>>>> Also, using vmaloc test driver i can trigger a kernel BUG: >>>>> >>>>> <snip> >>>>> [ 24.627577] kernel BUG at mm/vmalloc.c:1272! >>>> >>>> It seems that most tests indeed fail. Perhaps the vmalloc subsystem isn't >>>> very robust in face of fragmented virtual memory. What could be done to fix >>>> that? >>>> >>> Your patch is broken in context of checking "vend" when you try to >>> allocate next time after first attempt. Passed "vend" is different >>> there comparing what is checked later to figure out if an allocation >>> failed or not: >>> >>> <snip> >>> if (unlikely(addr == vend)) >>> goto overflow; >>> <snip> >> >> >> Thanks, I'll fix that. >> >>> >>>> >>>> In this patch, I could retry __alloc_vmap_area() with the whole region after >>>> failure of both [random, vend] and [vstart, random] but I'm not sure that >>>> would help much. Worth a try of course. >>>> >>> There is no need in your second [vstart, random]. If a first bigger range >>> has not been successful, the smaller one will never be success anyway. The >>> best way to go here is to repeat with real [vsart:vend], if it still fails >>> on a real range, then it will not be possible to accomplish an allocation >>> request with given parameters. >>> >>>> >>>> By the way, some of the tests in test_vmalloc.c don't check for vmalloc() >>>> failure, for example in full_fit_alloc_test(). >>>> >>> Where? >> >> Something like this: >> >> diff --git a/lib/test_vmalloc.c b/lib/test_vmalloc.c >> index 5cf2fe9aab9e..27e5db9a96b4 100644 >> --- a/lib/test_vmalloc.c >> +++ b/lib/test_vmalloc.c >> @@ -182,9 +182,14 @@ static int long_busy_list_alloc_test(void) >> if (!ptr) >> return rv; >> >> - for (i = 0; i < 15000; i++) >> + for (i = 0; i < 15000; i++) { >> ptr[i] = vmalloc(1 * PAGE_SIZE); >> >> + if (!ptr[i]) >> + goto leave; >> + } >> + >> > Hmm. That is for creating a long list of allocated areas before running > a test. For example if one allocation among 15 000 fails, some index will > be set to NULL. Later on after "leave" label vfree() will bypass NULL freeing. > > Either we have 15 000 extra elements or 10 000 does not really matter > and is considered as a corner case that is probably never happens. Yes, > you can simulate such precondition, but then a regular vmalloc()s will > likely also fails, thus the final results will be screwed up. I'd argue that if the allocations fail, the test should be aborted immediately since the results are not representative. -Topi > >> + >> for (i = 0; i < test_loop_count; i++) { >> ptr_1 = vmalloc(100 * PAGE_SIZE); >> if (!ptr_1) >> @@ -236,7 +241,11 @@ static int full_fit_alloc_test(void) >> >> for (i = 0; i < junk_length; i++) { >> ptr[i] = vmalloc(1 * PAGE_SIZE); >> + if (!ptr[i]) >> + goto error; >> junk_ptr[i] = vmalloc(1 * PAGE_SIZE); >> + if (!junk_ptr[i]) >> + goto error; >> } >> >> for (i = 0; i < junk_length; i++) >> @@ -256,8 +265,10 @@ static int full_fit_alloc_test(void) >> rv = 0; >> >> error: >> - for (i = 0; i < junk_length; i++) >> + for (i = 0; i < junk_length; i++) { >> vfree(ptr[i]); >> + vfree(junk_ptr[i]); >> + } >> >> vfree(ptr); >> vfree(junk_ptr); >> > Same here. > > -- > Vlad Rezki >
Powered by blists - more mailing lists