Re: Potential null pointer dereference in remap_vmalloc_range_partial()

[Vlastimil Babka <vbabka@suse.cz>]

On 5/3/20 8:03 AM, Dongyang Zhan wrote:
> Hi,
> 
> I am a security researcher. I found a potential bug in /mm/sparse.c. I hope you can help me to confirm it.
> 
> In Linux 4.10.17,

In general, the upstream community prioritises recent kernels, 4.10 is rather
old. Bugs from older kernels should be verified if they still apply to most
recent version. If they don't, it's a matter of identifying the fixing commit
and sending it to stable.

> remap_vmalloc_range_partial() in /mm/vmalloc.c does not check the validation of allocated memory 'page', which may cause a null pointer dereference bug.
> 
> int remap_vmalloc_range_partial(struct vm_area_struct *vma, unsigned long uaddr,
> 				void *kaddr, unsigned long size)
> {
> ...
>                 struct page *page = vmalloc_to_page(kaddr); //page is possible to be null
> 		int ret;
> 
> 		ret = vm_insert_page(vma, uaddr, page); //null pointer dereference of page
> 		if (ret)
> 			return ret;
> ...
> }
> Let us see vmalloc_to_page(kaddr) in /mm/vmalloc.c
> struct page *vmalloc_to_page(const void *vmalloc_addr)
> {
> 	unsigned long addr = (unsigned long) vmalloc_addr;
> 	struct page *page = NULL;
> 	pgd_t *pgd = pgd_offset_k(addr);
> 
> 	/*
> 	 * XXX we might need to change this if we add VIRTUAL_BUG_ON for
> 	 * architectures that do not vmalloc module space
> 	 */
> 	VIRTUAL_BUG_ON(!is_vmalloc_or_module_addr(vmalloc_addr));
> 
> 	if (!pgd_none(*pgd)) {
> 		...
> 	}
> 	return page;
> }
> We can find that page is possible to be NULL.

Current version of vmalloc_to_page() is even more explicit:

        if (pgd_none(*pgd))
                return NULL;

> Then, we can see this function vm_insert_page(vma, uaddr, page) in /mm/memory.c.
> int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
> 			struct page *page)
> {
> 	if (addr < vma->vm_start || addr >= vma->vm_end)
> 		return -EFAULT;
> 	if (!page_count(page)) //this function can trigger the bug.
> 		return -EINVAL;
> 	...
> }
> page_count() can be found in /include/linux/page_ref.h.
> static inline int page_count(struct page *page)
> {
> 	return atomic_read(&compound_head(page)->_refcount);
> }
> Directly using the 'page' pointer is not secure.

This all assumes remap_vmalloc_range_partial() was called with kaddr that
doesn't have the page table hierarchy populated. You say 'validation of
allocated memory 'page'' but page is not being allocated, it's looked up from
page tables. If it was valid vmalloc region, the page tables should be populated
and there's no NULL. To show a bug, you would need to show how
remap_vmalloc_range_partial() can be called with kaddr that's not a proper
vmalloc allocation.

HTH,
Vlastimil