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