Message-id: <54F060F5.10502@samsung.com>
Date: Fri, 27 Feb 2015 15:20:05 +0300
From: Andrey Ryabinin <a.ryabinin@samsung.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101
 Thunderbird/31.4.0
MIME-version: 1.0
To: Rusty Russell <rusty@rustcorp.com.au>,
        Andrew Morton <akpm@linux-foundation.org>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH] kasan, module,
 vmalloc: rework shadow allocation for modules
References: <1424281467-2593-1-git-send-email-a.ryabinin@samsung.com>
 <87pp96stmz.fsf@rustcorp.com.au> <54E5E355.9020404@samsung.com>
 <87fva1sajo.fsf@rustcorp.com.au> <54E6E684.4070806@samsung.com>
 <87vbithw4b.fsf@rustcorp.com.au> <54EC7563.8090801@samsung.com>
 <874mqamrri.fsf@rustcorp.com.au> <54ED803F.6040308@samsung.com>
 <87vbiplarm.fsf@rustcorp.com.au>
In-reply-to: <87vbiplarm.fsf@rustcorp.com.au>
Content-type: text/plain; charset=utf-8
Content-transfer-encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6150
Lines: 158

On 02/26/2015 04:30 AM, Rusty Russell wrote:
> Andrey Ryabinin <a.ryabinin@samsung.com> writes:
>> On 02/25/2015 09:25 AM, Rusty Russell wrote:
>>> Andrey Ryabinin <a.ryabinin@samsung.com> writes:
>>>> On 02/23/2015 11:26 AM, Rusty Russell wrote:
>>>>> Andrey Ryabinin <a.ryabinin@samsung.com> writes:
>>>>>> On 02/20/2015 03:15 AM, Rusty Russell wrote:
>>>>>>> Andrey Ryabinin <a.ryabinin@samsung.com> writes:
>>>>>>>> On 02/19/2015 02:10 AM, Rusty Russell wrote:
>>>>>>>>> This is not portable.  Other archs don't use vmalloc, or don't use
>>>>>>>>> (or define) MODULES_VADDR.  If you really want to hook here, you'd
>>>>>>>>> need a new flag (or maybe use PAGE_KERNEL_EXEC after an audit).
>>>>>>>>>
>>>>>>>>
>>>>>>>> Well, instead of explicit (addr >= MODULES_VADDR && addr < MODULES_END)
>>>>>>>> I could hide this into arch-specific function: 'kasan_need_to_allocate_shadow(const void *addr)'
>>>>>>>> or make make all those functions weak and allow arch code to redefine them.
>>>>>>>
>>>>>>> That adds another layer of indirection.  And how would the caller of
>>>>>>> plain vmalloc() even know what to return?
>>>>>>>
>>>>>>
>>>>>> I think I don't understand what do you mean here. vmalloc() callers shouldn't know
>>>>>> anything about kasan/shadow.
>>>>>
>>>>> How else would kasan_need_to_allocate_shadow(const void *addr) work for
>>>>> architectures which don't have a reserved vmalloc region for modules?
>>>>>
>>>>
>>>>
>>>> I think I need to clarify what I'm doing.
>>>>
>>>> Address sanitizer algorithm in short:
>>>> -------------------------------------
>>>> Every memory access is transformed by the compiler in the following way:
>>>>
>>>> Before:
>>>> 	*address = ...;
>>>>
>>>> after:
>>>>
>>>> 	if (memory_is_poisoned(address)) {
>>>> 		report_error(address, access_size);
>>>> 	}
>>>> 	*address = ...;
>>>>
>>>> where memory_is_poisoned():
>>>> 	bool memory_is_poisoned(unsigned long addr)
>>>> 	{
>>>>         	s8 shadow_value = *(s8 *)kasan_mem_to_shadow((void *)addr);
>>>> 	        if (unlikely(shadow_value)) {
>>>>         	        s8 last_accessible_byte = addr & KASAN_SHADOW_MASK;
>>>>                 	return unlikely(last_accessible_byte >= shadow_value);
>>>> 	        }
>>>> 	        return false;
>>>> 	}
>>>> --------------------------------------
>>>>
>>>> So shadow memory should be present for every accessible address in kernel
>>>> otherwise it will be unhandled page fault on reading shadow value.
>>>>
>>>> Shadow for vmalloc addresses (on x86_64) is readonly mapping of one zero page.
>>>> Zero byte in shadow means that it's ok to access to that address.
>>>> Currently we don't catch bugs in vmalloc because most of such bugs could be caught
>>>> in more simple way with CONFIG_DEBUG_PAGEALLOC.
>>>> That's why we don't need RW shadow for vmalloc, it just one zero page that readonly
>>>> mapped early on boot for the whole [kasan_mem_to_shadow(VMALLOC_START, kasan_mem_to_shadow(VMALLOC_END)] range
>>>> So every access to vmalloc range assumed to be valid.
>>>>
>>>> To catch out of bounds accesses in global variables we need to fill shadow corresponding
>>>> to variable's redzone with non-zero (negative) values.
>>>> So for kernel image and modules we need a writable shadow.
>>>>
>>>> If some arch don't have separate address range for modules and it uses general vmalloc()
>>>> shadow for vmalloc should be writable, so it means that shadow has to be allocated
>>>> for every vmalloc() call.
>>>>
>>>> In such arch kasan_need_to_allocate_shadow(const void *addr) should return true for every vmalloc address:
>>>> bool kasan_need_to_allocate_shadow(const void *addr)
>>>> {
>>>> 	return addr >= VMALLOC_START && addr < VMALLOC_END;
>>>> }
>>>
>>> Thanks for the explanation.
>>>
>>>> All above means that current code is not very portable.
>>>> And 'kasan_module_alloc(p, size) after module alloc' approach is not portable
>>>> too. This won't work for arches that use [VMALLOC_START, VMALLOC_END] addresses for modules,
>>>> because now we need to handle all vmalloc() calls.
>>>
>>> I'm confused.  That's what you do now, and it hasn't been a problem,
>>> has it?  The problem is on the freeing from interrupt context...
>>>
>>
>> It's not problem now. It's only about portability.
> 
> Your first patch in this conversation says "Current approach in handling
> shadow memory for modules is broken."
> 

Sorry, my last answer was even more confusing.
You are right, the main problem is on the freeing form interrupts.

I meant that this:

> This won't work for arches that use [VMALLOC_START, VMALLOC_END] addresses for modules,
> because now we need to handle all vmalloc() calls.

is not a problem for now.


>>> #define VM_KASAN		0x00000080      /* has shadow kasan map */
>>>
>>> Set that in kasan_module_alloc():
>>>
>>>         if (ret) {
>>>                 struct vm_struct *vma = find_vm_area(addr);
>>>
>>>                 BUG_ON(!vma);
>>>                 /* Set VM_KASAN so vfree() can free up shadow. */
>>>                 vma->flags |= VM_KASAN;
>>>         }
>>>
>>> And check that in __vunmap():
>>>
>>>         if (area->flags & VM_KASAN)
>>>                 kasan_module_free(addr);
>>>
>>> That is portable, and is actually a fairly small patch on what you
>>> have at the moment.
>>>
>>> What am I missing?
>>>
>>
>> That is not portable.
>> Architectures that don't have separate region for modules should allocate shadow
>> for every vmalloc() call, not only for modules.
> 
> OK, I didn't appreciate that.  But couldn't you still use the "R/O
> shared zero page shadow" for vmalloc, and have kasan_module_alloc()
> simply replace the pages with r/w ones (and kasan_module_free()
> would have to remove it again).
> 
>> Actually I'm fine with what you are proposing here. I think that portability issues could be fixed
>> latter when this will become a real problem.
> 
> OK.
> 
> Thanks for your patience!
> Rusty.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/