Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4] mm/vmalloc: randomize vmalloc() allocations
To:     Uladzislau Rezki <urezki@gmail.com>
Cc:     linux-hardening@vger.kernel.org, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Andy Lutomirski <luto@kernel.org>,
        Jann Horn <jannh@google.com>,
        Kees Cook <keescook@chromium.org>,
        Linux API <linux-api@vger.kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Mike Rapoport <rppt@kernel.org>
References: <20210309135757.5406-1-toiwoton@gmail.com>
 <20210314172312.GA2085@pc638.lan>
 <ba975607-9493-c78c-bbd8-6a85573114d1@gmail.com>
 <20210315153510.GA1865@pc638.lan>
From:   Topi Miettinen <toiwoton@gmail.com>
Message-ID: <4649f69d-b7cd-d1a6-26e0-9b8bf3b17df5@gmail.com>
Date:   Mon, 15 Mar 2021 18:23:37 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.0
MIME-Version: 1.0
In-Reply-To: <20210315153510.GA1865@pc638.lan>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;
+       }
+
+
         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);

-Topi