From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751001AbdEERSr (ORCPT ); Fri, 5 May 2017 13:18:47 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:31568 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750777AbdEERSq (ORCPT ); Fri, 5 May 2017 13:18:46 -0400 Subject: Re: [PATCH] kthread: fix use-after-free if kthread fork fails To: Oleg Nesterov References: <20170505162034.4338-1-vegard.nossum@oracle.com> <20170505164428.GA500@redhat.com> Cc: linux-kernel@vger.kernel.org, Greg Kroah-Hartman , Frederic Weisbecker , Jamie Iles , Peter Zijlstra , Thomas Gleixner , Andy Lutomirski From: Vegard Nossum Message-ID: <939ac33e-ebe1-2b1e-425f-aced558ad5e3@oracle.com> Date: Fri, 5 May 2017 19:17:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170505164428.GA500@redhat.com> Content-Type: multipart/mixed; boundary="------------F57869843C98C993B2F07691" X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is a multi-part message in MIME format. --------------F57869843C98C993B2F07691 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 05/05/17 18:44, Oleg Nesterov wrote: > On 05/05, Vegard Nossum wrote: >> >> If a kthread forks (e.g. usermodehelper since commit 1da5c46fa965) but >> fails in copy_process() between calling dup_task_struct() and setting >> p->set_child_tid, then the value of p->set_child_tid will be inherited >> from the parent and get prematurely freed by free_kthread_struct(). > > Aaah... thanks! > >> --- a/kernel/fork.c >> +++ b/kernel/fork.c >> @@ -518,6 +518,13 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node) >> atomic_set(&tsk->stack_refcount, 1); >> #endif >> >> + /* >> + * Forking kthreads (e.g. usermodehelper) should not inherit this >> + * field since it's a pointer to a 'struct kthread' which is not >> + * reference counted. >> + */ >> + tsk->set_child_tid = NULL; >> + > > Can't we just move both > > p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL; > /* > * Clear TID on mm_release()? > */ > p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? child_tidptr : NULL; > > lines here? clone_flags is not available in dup_task_struct(), but we could move those lines higher in copy_process(). The reason we didn't do it was that we thought it was a little fragile/unobvious that this has to happen before free_task() is called and that it was safer to clear it in dup_task_struct() (which also contains zeroing of other fields). The newly attached patch has been tested and seems to work, if you prefer it. Vegard --------------F57869843C98C993B2F07691 Content-Type: text/x-patch; name="set_child_tid.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="set_child_tid.patch" diff --git a/kernel/fork.c b/kernel/fork.c index fbdc29365b83..c52e22fdf7ca 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1561,6 +1561,18 @@ static __latent_entropy struct task_struct *copy_process( if (!p) goto fork_out; + /* + * This _must_ happen before we call free_task(), i.e. before we jump + * to any of the bad_fork_* labels. This is to avoid freeing + * p->set_child_tid which is (ab)used as a kthread's data pointer for + * kernel threads (PF_KTHREAD). + */ + p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL; + /* + * Clear TID on mm_release()? + */ + p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? child_tidptr : NULL; + ftrace_graph_init_task(p); rt_mutex_init_task(p); @@ -1727,11 +1739,6 @@ static __latent_entropy struct task_struct *copy_process( } } - p->set_child_tid = (clone_flags & CLONE_CHILD_SETTID) ? child_tidptr : NULL; - /* - * Clear TID on mm_release()? - */ - p->clear_child_tid = (clone_flags & CLONE_CHILD_CLEARTID) ? child_tidptr : NULL; #ifdef CONFIG_BLOCK p->plug = NULL; #endif --------------F57869843C98C993B2F07691--