From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=FSL_HELO_FAKE, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,USER_AGENT_NEOMUTT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CA6E8C4360F for ; Wed, 6 Mar 2019 20:39:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9AC4620684 for ; Wed, 6 Mar 2019 20:39:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726156AbfCFUjl (ORCPT ); Wed, 6 Mar 2019 15:39:41 -0500 Received: from youngberry.canonical.com ([91.189.89.112]:53659 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726474AbfCFUjk (ORCPT ); Wed, 6 Mar 2019 15:39:40 -0500 Received: from mail-ed1-f69.google.com ([209.85.208.69]) by youngberry.canonical.com with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.76) (envelope-from ) id 1h1dKD-00046F-QQ for stable@vger.kernel.org; Wed, 06 Mar 2019 20:39:37 +0000 Received: by mail-ed1-f69.google.com with SMTP id f2so6812916edm.18 for ; Wed, 06 Mar 2019 12:39:37 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=xdpZkT7R0+dWNs314gvphoexyR7vBHsfYtrJAa8MRmA=; b=kmCLbIvBgooYhCn2U/gqlJ53JQ2icsBaMQJLhEg4lCjXHNO/0PIOiCSEkX82U0N9ML H5u5Hngo7TDTHo7udwoReXcPiUDwSLv/41FdyCcN4Cy4p2vj3dLWQ8vXup5qhHIR6eks J+qxbGUzY7mvf82jfHzbJ/SiC88qAjMW2BIYBtgrthNacbIGTWS6Lsc5vkBQ7TdHfd/+ e35DCpBWtvrSln6KTx1nzICxVKBJ/iJUQyNx9cbP2ZeNeGmrPwsa52cF07MXZVMsZeig mPtobzoy6b4h8SwCZYpcNTn2veI8PkYgOl1iG1SKFTXBONNOER4TBkBaMxqVAavWtJqg KbJA== X-Gm-Message-State: APjAAAXOu/tgSE+bdEtvUamQKOOo/p5N8yuexYOhtPoYH0h3svpKniPo z2ud8RBxwoQ8gCpnQYtPVb4r9w2R4JjJk33i8aFoX/jwMSMceXBv5gaf4DTneOaBDlrhlh+9vMB tOuutAVgVuPwyLEsfo/gJeGDM+vlkGqAZ1w== X-Received: by 2002:a50:8978:: with SMTP id f53mr25244698edf.212.1551904776968; Wed, 06 Mar 2019 12:39:36 -0800 (PST) X-Google-Smtp-Source: APXvYqy+1nPWVN98CbPLZzXydiux0qZC3nISnqul3y8cj1fN2Y2APsLGaCXQo8/DXIKtDaT2FdkvrQ== X-Received: by 2002:a50:8978:: with SMTP id f53mr25244678edf.212.1551904776571; Wed, 06 Mar 2019 12:39:36 -0800 (PST) Received: from gmail.com ([2a02:8109:b6c0:76e:3ddf:44fc:d22b:95ce]) by smtp.gmail.com with ESMTPSA id x36sm725660eda.94.2019.03.06.12.39.35 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Wed, 06 Mar 2019 12:39:36 -0800 (PST) From: Christian Brauner X-Google-Original-From: Christian Brauner Date: Wed, 6 Mar 2019 21:39:35 +0100 To: Tycho Andersen Cc: Kees Cook , linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags Message-ID: <20190306203932.im7lkblvmd227td3@gmail.com> References: <20190306201413.14153-1-tycho@tycho.ws> <20190306201413.14153-2-tycho@tycho.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190306201413.14153-2-tycho@tycho.ws> User-Agent: NeoMutt/20180716 Sender: stable-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: stable@vger.kernel.org On Wed, Mar 06, 2019 at 01:14:13PM -0700, Tycho Andersen wrote: > As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict, > because they both return positive values, one in the case of success and > one in the case of error. So, let's disallow both of these flags together. > > While this is technically a userspace break, all the users I know of are > still waiting on me to land this feature in libseccomp, so I think it'll be > safe. Also, at present my use case doesn't require TSYNC at all, so this > isn't a big deal to disallow. If someone wanted to support this, a path > forward would be to add a new flag like > TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but > the use cases are so different I don't see it really happening. > > Finally, it's worth noting that this does actually fix a UAF issue: at the end > of seccomp_set_mode_filter(), we have: > > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > if (ret < 0) { > listener_f->private_data = NULL; > fput(listener_f); > put_unused_fd(listener); > } else { > fd_install(listener, listener_f); > ret = listener; > } > } > out_free: > seccomp_filter_free(prepared); > > But if ret > 0 because TSYNC raced, we'll install the listener fd and then free > the filter out from underneath it, causing a UAF when the task closes it or > dies. This patch also switches the condition to be simply if (ret), so that > if someone does add the flag mentioned above, they won't have to remember > to fix this too. > > Signed-off-by: Tycho Andersen > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") > CC: stable@vger.kernel.org # v5.0+ > --- > kernel/seccomp.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index d0d355ded2f4..79bada51091b 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter) > * > * Caller must be holding current->sighand->siglock lock. > * > - * Returns 0 on success, -ve on error. > + * Returns 0 on success, -ve on error, or > + * - in TSYNC mode: the pid of a thread which was either not in the correct > + * seccomp mode or did not have an ancestral seccomp filter > + * - in NEW_LISTENER mode: the fd of the new listener > */ > static long seccomp_attach_filter(unsigned int flags, > struct seccomp_filter *filter) > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags, > if (flags & ~SECCOMP_FILTER_FLAG_MASK) > return -EINVAL; > > + /* > + * In the successful case, NEW_LISTENER returns the new listener fd. > + * But in the failure case, TSYNC returns the thread that died. If you > + * combine these two flags, there's no way to tell whether something > + * succeded or failed. So, let's disallow this combination. > + */ > + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && > + (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER)) > + return -EINVAL; May license a manpage entry that this makes it potentially unsafe to use with multiple threads. But I don't see a use-case for this right now so it looks sane to me. :) (Though one simple question below.) Acked-by: Christian Brauner > + > /* Prepare the new filter before holding any locks. */ > prepared = seccomp_prepare_user_filter(filter); > if (IS_ERR(prepared)) > @@ -1302,7 +1315,7 @@ static long seccomp_set_mode_filter(unsigned int flags, > mutex_unlock(¤t->signal->cred_guard_mutex); > out_put_fd: > if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) { > - if (ret < 0) { > + if (ret) { Why that change but keep checking if (ret < 0) further up? > listener_f->private_data = NULL; > fput(listener_f); > put_unused_fd(listener); > -- > 2.19.1 >