From patchwork Wed Apr 24 16:37:56 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 10915211 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id A12E5922 for ; Wed, 24 Apr 2019 16:38:19 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8E61C28AB6 for ; Wed, 24 Apr 2019 16:38:19 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 823D528AE6; Wed, 24 Apr 2019 16:38:19 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0904628AB6 for ; Wed, 24 Apr 2019 16:38:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731345AbfDXQiR (ORCPT ); Wed, 24 Apr 2019 12:38:17 -0400 Received: from mail-pf1-f193.google.com ([209.85.210.193]:46164 "EHLO mail-pf1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732609AbfDXQiH (ORCPT ); Wed, 24 Apr 2019 12:38:07 -0400 Received: by mail-pf1-f193.google.com with SMTP id 9so9572327pfj.13 for ; Wed, 24 Apr 2019 09:38:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=0PO8cqNNrsc0erE2qFybiWt9ZeKPOIyq2aL7nG116E4=; b=lKNPpcuRi/jRmdBX5Un4UMoF9evOPzRrfzwK/AP3gJqsY1umqamPoaG6/MpT9jgXgL CrgNPHJUsLzhAZiAhiHCvJtf21B0JouJPSD21ExLQPJmu4nu6AQS/j+fsZPdnJ09YX/C nps/moWtLTGWg3c+gBXol4F31ZKIA9SeJ4eNM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=0PO8cqNNrsc0erE2qFybiWt9ZeKPOIyq2aL7nG116E4=; b=fxbiz6IkpduH49FssJSzwXRe0x1PWLPic0cgucgbU4UNSO9kPPJUTGCTE0b1XF6ssf 7Q6CPglkEGeu80nF3f1QAyLEaP4A3qnRi8TrdSXlCKgg82WeHqgulPdEng64n1HDw682 9RrgDNjU9SgjVuwE+nSKFMSkNUpkfnDzhiWoJTpdilaExX2fMmtVF7TUP1jEFK9h1pud aAMHfA0zGpBO9RPEjBDLv/3YWEQscm/EUa4dnenVGXJt3U0K4jdj5jSeBwNE0LMP9D7F G3kQTD8U0FNoz4Dk1Q6NQLIoUQOHD7l794MtMEZOqD1Mi+iCCf7IS+WZMNZgZBOaltcM TCUQ== X-Gm-Message-State: APjAAAXXYVVmxKdTyFKAboXypOCVIzEUX8EEEfpNgwkM0cOl0WUI+dik McVw5uBFKltr9QJI1rEo72mi1w== X-Google-Smtp-Source: APXvYqw+WRueRcdgaAkh3mJqglMzdxBa+Azdx/uYhSx50VoUTeBsE10FaDB+/ehC5KQ8hMhj2SXAKQ== X-Received: by 2002:a65:4343:: with SMTP id k3mr160190pgq.384.1556123886584; Wed, 24 Apr 2019 09:38:06 -0700 (PDT) Received: from www.outflux.net (173-164-112-133-Oregon.hfc.comcastbusiness.net. [173.164.112.133]) by smtp.gmail.com with ESMTPSA id b7sm38220062pfj.67.2019.04.24.09.38.04 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 24 Apr 2019 09:38:04 -0700 (PDT) From: Kees Cook To: Tycho Andersen Cc: Kees Cook , stable@vger.kernel.org, James Morris , Andy Lutomirski , Will Drewry , linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH 2/2] seccomp: Make NEW_LISTENER and TSYNC flags exclusive Date: Wed, 24 Apr 2019 09:37:56 -0700 Message-Id: <20190424163756.40001-3-keescook@chromium.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20190424163756.40001-1-keescook@chromium.org> References: <20190424163756.40001-1-keescook@chromium.org> Sender: linux-kselftest-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kselftest@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP From: Tycho Andersen 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. Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace") CC: stable@vger.kernel.org # v5.0+ Signed-off-by: Tycho Andersen Signed-off-by: Kees Cook Acked-by: James Morris --- kernel/seccomp.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/kernel/seccomp.c b/kernel/seccomp.c index df27e499956a..3582eeb59893 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -502,7 +502,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) @@ -1258,6 +1261,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 + * succeeded or failed. So, let's disallow this combination. + */ + if ((flags & SECCOMP_FILTER_FLAG_TSYNC) && + (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER)) + return -EINVAL; + /* Prepare the new filter before holding any locks. */ prepared = seccomp_prepare_user_filter(filter); if (IS_ERR(prepared)) @@ -1304,7 +1317,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) { listener_f->private_data = NULL; fput(listener_f); put_unused_fd(listener);