ptrace: PTRACE_GETFDPIC: fix the unsafe usage of child->mm
in [Kernel]
|
Prev: [PATCH v3 3/3] dm: lookup devices by path with name_to_dev_t
Next: [PATCH resent twice 2/3] vgaarb: use MIT license
From: Oleg Nesterov on 24 May 2010 11:20 On 05/24, David Howells wrote: > > Oleg Nesterov <oleg(a)redhat.com> wrote: > > > Now that Mike Frysinger unified the FDPIC ptrace code, we can fix > > the unsafe usage of child->mm in ptrace_request(PTRACE_GETFDPIC). > > > > We have the reference to task_struct, and ptrace_check_attach() > > verified the tracee is stopped. But nothing can protect from > > SIGKILL after that, we must not assume child->mm != NULL. > > > > Signed-off-by: Oleg Nesterov <oleg(a)redhat.com> > > Acked-by: David Howells <dhowells(a)redhat.com> > > Does it make sense to move the call to get_task_mm() up to sys_ptrace() since > several ptrace functions use it? The mm pointer could then be handed down the > ptrace hierarchy. You mean, pass it to arch_ptrace() ? grep, grep, grep. I guess I understand you. We have more unsafe code like this in arch/*/kernel/ptrace.c. Of course, it can be fixed without doing get_task_mm() in sys_ptrace(), but perhaps it would be more clean to do what you suggest. Roland, what do you think? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Roland McGrath on 24 May 2010 19:50 > You mean, pass it to arch_ptrace() ? > > grep, grep, grep. I guess I understand you. We have more unsafe code > like this in arch/*/kernel/ptrace.c. Of course, it can be fixed without > doing get_task_mm() in sys_ptrace(), but perhaps it would be more clean > to do what you suggest. > > Roland, what do you think? The mm pointer is only used by these uncommon ptrace operations that exist only in certain unusual arch's (and they're all ill-advised old arch ptrace ABI additions, at that). It doesn't seem wise to pay the overhead for get_task_mm()/mmput() on every ptrace call, 99.44% of which don't use it (and 100% on 90% of machines). If you were to make any change to the signature of arch_ptrace() it should be one big change to use a struct ptrace_params or suchlike, also passed down to ptrace_request(). Then any future needs to pass around more information won't require changing the code in all the arch code that doesn't look at the new parameter. Thanks, Roland -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 25 May 2010 06:30 On 05/25, David Howells wrote: > > Roland McGrath <roland(a)redhat.com> wrote: > > > The mm pointer is only used by these uncommon ptrace operations > > Like PEEKTEXT and POKETEXT? They use access_process_vm(). According to grep, mm is only use to read a couple of members. Perhaps can even add the simple helper struct mm_xxx { unsigned long start_code, end_code, start_data, end_data; ... some more ... }; int get_mm_xxx(struct task_struct *tracee, struct mm_xxx *mm_xxx) { struct mm_struct *mm = get_task_mm(tracee); ... } Except: - arch/um/kernel/ptrace.c PTRACE_SWITCH_MM does something really strange - arch/blackfin/kernel/ptrace.c:is_user_addr_valid() needs mmap_sem around find_vma() The lockless access to mm->context.sram_list doesn't look safe to me. If we add get_task_mm() - this protects us against destroy_context() only. What is the tracee's sub-thread does sys_sram_alloc() or sys_sram_free() in parallel? Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 25 May 2010 08:40 On 05/25, David Howells wrote: > > Oleg Nesterov <oleg(a)redhat.com> wrote: > > > > Like PEEKTEXT and POKETEXT? > > > > They use access_process_vm(). > > Which needs to get the mm: > > int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write) > { > struct vm_area_struct *vma; > struct mm_struct *mm; > > if (addr + len < addr) > return 0; > > mm = get_task_mm(tsk); Yes sure, But I do not think it makes any sense to change the signature of access_process_vm() as well, it has a lot of callers. And it is complex, it does get_user_pages(). Compared to that get_task_mm() inside of access_process_vm() is nothing. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Oleg Nesterov on 26 May 2010 08:50
On 05/25, Mike Frysinger wrote: > > On Tue, May 25, 2010 at 06:23, Oleg Nesterov wrote: > > � � � �- arch/blackfin/kernel/ptrace.c:is_user_addr_valid() > > � � � � �needs mmap_sem around find_vma() > > > > � � � � �The lockless access to mm->context.sram_list doesn't look > > � � � � �safe to me. > > > > � � � � �If we add get_task_mm() - this protects us against > > � � � � �destroy_context() only. What is the tracee's sub-thread > > � � � � �does sys_sram_alloc() or sys_sram_free() in parallel? > > i dont believe there are any code paths in UP systems where this would > be a practical problem because sram_list is only updated by syscalls > from userspace. Yes sure, UP && !PREEMPT is safe. > we probably should add proper locking to this > structure though. Agreed. I'll try to make the trivial patch tomorrow. I think we can just use mm->mmap_sem, is_user_addr_valid() needs this lock for find_vma() anyway. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |