linux-kernel - Re: [RFC] [PATCH] Pre-emption control for userspace

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 04 Mar 2014 13:14:51 -0700
From:	Khalid Aziz <khalid.aziz@...cle.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
	peterz@...radead.org, akpm@...ux-foundation.org,
	andi.kleen@...el.com, rob@...dley.net, viro@...iv.linux.org.uk,
	venki@...gle.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC] [PATCH] Pre-emption control for userspace

On 03/04/2014 12:03 PM, Oleg Nesterov wrote:
> On 03/04, Khalid Aziz wrote:
>>
>> On 03/04/2014 06:56 AM, Oleg Nesterov wrote:
>>> Hmm. In fact I think do_exit() should crash after munmap? ->mmap_state
>>> should be NULL ?? Perhaps I misread this patch completely...
>>
>> do_exit() unmaps mmap_state->uaddr, and frees up mmap_state->kaddr and
>> mmap_state. mmap_state should not be NULL after unmap.
>
> Can't understand... do_exit() does:
>
> 	+#if CONFIG_SCHED_PREEMPT_DELAY
> 	+       if (tsk->sched_preempt_delay.mmap_state) {
> 	+               sys_munmap((unsigned long)
> 	+                       tsk->sched_preempt_delay.mmap_state->uaddr, PAGE_SIZE);
> 	+               vfree(tsk->sched_preempt_delay.mmap_state->kaddr);
> 	+               kfree(tsk->sched_preempt_delay.mmap_state);
> 	
> sys_munmap() (which btw should not be used) obviously unmaps that
> vma and vma_ops()->close() should be called.
>
> close_preempt_delay_vmops() does:
>
> 	state->task->sched_preempt_delay.mmap_state = NULL;
>
> vfree(tsk->sched_preempt_delay.mmap_state->kaddr) above will try to
> dereference .mmap_state == NULL.
>
> IOW, I think that with this patch this trivial program
>
> 	int main(void)
> 	{
> 		fd = open("/proc/self/task/$TID/sched_preempt_delay", O_RDWR);
> 		mmap(NULL, 4096, PROT_READ,MAP_SHARED, fd, 0);
> 		return 0;
> 	}
>
> should crash the kernel.
>
>>>> +	state->page = page;
>>>> +	state->kaddr = kaddr;
>>>> +	state->uaddr = (void *)vma->vm_start;
>>>
>>> This is used by do_exit(). But ->vm_start can be changed by mremap() ?
>>>
>>>
>>> Hmm. And mremap() can do vm_ops->close() too. But the new vma will
>>> have the same vm_ops/vm_private_data, so exit_mmap() will try to do
>>> this again... Perhaps I missed something, but I bet this all can't be
>>> right.
>>
>> Would you say sys_munmap() of mmap_state->uaddr is not even needed since
>> exit_mm() will do this any way further down in do_exit()?
>
> No.
>
> I meant:
>
> 	1. mremap() can move this vma, so do_exit() can't trust ->uaddr
>
> 	2. Even worse, mremap() itself is not safe. It can do ->close()
> 	   too and create the new vma with the same vm_ops. Another
> 	   unmap from (say) exit_mm() won't be happy.

I agree this looks like a potential spot for trouble. I was asking if 
removing sys_munmap() of uaddr from do_exit() solves both of the above 
problems? You have convinced me this sys_munmap() I added is unnecessary.

>
>>>> +	vma->vm_flags |= VM_DONTCOPY | VM_DONTEXPAND | VM_SHARED | VM_WRITE;
>>>
>>> This probably also needs VM_IO, to protect from madvise(MADV_DOFORK).
>>
>> Yes, you are right. I will add that.
>>
>>> VM_SHARED/VM_WRITE doesn't look right.
>>
>> VM_SHARED is wrong but VM_WRITE is needed I think since the thread will
>> write to the mmap'd page to signal to request preemption delay.
>
> But ->mmap() should not set VM_WRITE if application does mmap(PROT_READ) ?
> VM_WRITE-or-not should be decided by do_mmap_pgoff/mprotect, ->mmap()
> should not play with this bit.
>

Ah, I see. This makes sense. I will remove it.

Thanks,
Khalid


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