From patchwork Tue Jun 16 03:25:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 11606323 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id E4E29912 for ; Tue, 16 Jun 2020 03:26:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C951D207E8 for ; Tue, 16 Jun 2020 03:26:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="FKACMKyf" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727070AbgFPD0O (ORCPT ); Mon, 15 Jun 2020 23:26:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44690 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727003AbgFPDZx (ORCPT ); Mon, 15 Jun 2020 23:25:53 -0400 Received: from mail-pl1-x642.google.com (mail-pl1-x642.google.com [IPv6:2607:f8b0:4864:20::642]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 61F59C0085C6 for ; Mon, 15 Jun 2020 20:25:37 -0700 (PDT) Received: by mail-pl1-x642.google.com with SMTP id j4so4510328plk.3 for ; Mon, 15 Jun 2020 20:25:37 -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 :mime-version:content-transfer-encoding; bh=0XLeJlm9EgjXcakcD9qCa51hLblsobf6+bsB3dZc0i8=; b=FKACMKyfxUA7EW9AFaoKVvRC5y/1zAscVNsROkN95jbzPz/DVctU6B9b39AAvKij7N 6Nlikz3HjI7F5mqdklSF71vSDdkiuk/lo6fE6XK2BMW71zU3jrwvPABfdtnLEM9oR5Bn 531elB+kI1SaW2n2BDWmttY4VIZSDBG5Z5y28= 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:mime-version:content-transfer-encoding; bh=0XLeJlm9EgjXcakcD9qCa51hLblsobf6+bsB3dZc0i8=; b=No9j1f6CbeJCgCM06g6KQoa66js7cMhzSzu8ZJhrUGZBxuoPdEpwELsTNlrEfX8yAg biz8sJHkSzUotzXwKZDqHcwOLg1ydQDNyFX3+6IEZ15dOv7uhuvYCqH7ocZyglwgprBJ ImPGDENW+pO0FZJSyXEa+Mt2D/O9r4XcANhdAPwCuhwFQKoPE3PKeOePL4as3mrPb5C4 BQf7KF37VB3d4OlnNJ0Hzl44xrT2JXtQVXM8opamf3LSotEkceKDrld0lyg0CkZZB4TO oazbz9npQAkH0vyLN7iRqRvmz8+ABEXuUl2HSx0KWqCXFC3l6mFRykgFi1/EbiIZytAV D6ig== X-Gm-Message-State: AOAM532jR6HCtB1eywpUb1iunw4m3xthNJhNJUyflw979nEdRV2Q09hK JkOeJcnOJ5M2NfBLz5I1KH5jqQ== X-Google-Smtp-Source: ABdhPJwVGDqjGiwBmdUw+0CM58qrM4XZkDqMs17lV5U6dITKbuVhjYUHCQGtjBUBVlvKZE+GWSFt+Q== X-Received: by 2002:a17:90a:634a:: with SMTP id v10mr851059pjs.50.1592277937337; Mon, 15 Jun 2020 20:25:37 -0700 (PDT) Received: from www.outflux.net (smtp.outflux.net. [198.145.64.163]) by smtp.gmail.com with ESMTPSA id 140sm15093613pfy.95.2020.06.15.20.25.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 15 Jun 2020 20:25:32 -0700 (PDT) From: Kees Cook To: linux-kernel@vger.kernel.org Cc: Kees Cook , Sargun Dhillon , Christian Brauner , "David S. Miller" , Christoph Hellwig , Tycho Andersen , Jakub Kicinski , Alexander Viro , Aleksa Sarai , Matt Denton , Jann Horn , Chris Palmer , Robert Sesek , Giuseppe Scrivano , Greg Kroah-Hartman , Andy Lutomirski , Will Drewry , Shuah Khan , netdev@vger.kernel.org, containers@lists.linux-foundation.org, linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kselftest@vger.kernel.org Subject: [PATCH v4 10/11] seccomp: Switch addfd to Extensible Argument ioctl Date: Mon, 15 Jun 2020 20:25:23 -0700 Message-Id: <20200616032524.460144-11-keescook@chromium.org> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20200616032524.460144-1-keescook@chromium.org> References: <20200616032524.460144-1-keescook@chromium.org> MIME-Version: 1.0 Sender: linux-kselftest-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kselftest@vger.kernel.org This patch is based on discussions[1] with Sargun Dhillon, Christian Brauner, and David Laight. Instead of building size into the addfd structure, make it a function of the ioctl command (which is how sizes are normally passed to ioctls). To support forward and backward compatibility, just mask out the direction and size, and match everything. The size (and any future direction) checks are done along with copy_struct_from_user() logic. Also update the selftests to check size bounds. [1] https://lore.kernel.org/lkml/20200612104629.GA15814@ircssh-2.c.rugged-nimbus-611.internal Signed-off-by: Kees Cook --- include/uapi/linux/seccomp.h | 2 - kernel/seccomp.c | 21 ++++++---- tools/testing/selftests/seccomp/seccomp_bpf.c | 40 ++++++++++++++++--- 3 files changed, 49 insertions(+), 14 deletions(-) diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h index c347160378e5..473a61695ac3 100644 --- a/include/uapi/linux/seccomp.h +++ b/include/uapi/linux/seccomp.h @@ -118,7 +118,6 @@ struct seccomp_notif_resp { /** * struct seccomp_notif_addfd - * @size: The size of the seccomp_notif_addfd structure * @id: The ID of the seccomp notification * @flags: SECCOMP_ADDFD_FLAG_* * @srcfd: The local fd number @@ -126,7 +125,6 @@ struct seccomp_notif_resp { * @newfd_flags: The O_* flags the remote FD should have applied */ struct seccomp_notif_addfd { - __u64 size; __u64 id; __u32 flags; __u32 srcfd; diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 3c913f3b8451..9660abf91135 100644 --- a/kernel/seccomp.c +++ b/kernel/seccomp.c @@ -1292,17 +1292,17 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter, } static long seccomp_notify_addfd(struct seccomp_filter *filter, - struct seccomp_notif_addfd __user *uaddfd) + struct seccomp_notif_addfd __user *uaddfd, + unsigned int size) { struct seccomp_notif_addfd addfd; struct seccomp_knotif *knotif; struct seccomp_kaddfd kaddfd; - u64 size; int ret; - ret = get_user(size, &uaddfd->size); - if (ret) - return ret; + /* 24 is original sizeof(struct seccomp_notif_addfd) */ + if (size < 24 || size >= PAGE_SIZE) + return -EINVAL; ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size); if (ret) @@ -1391,6 +1391,7 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, struct seccomp_filter *filter = file->private_data; void __user *buf = (void __user *)arg; + /* Fixed-size ioctls */ switch (cmd) { case SECCOMP_IOCTL_NOTIF_RECV: return seccomp_notify_recv(filter, buf); @@ -1398,11 +1399,17 @@ static long seccomp_notify_ioctl(struct file *file, unsigned int cmd, return seccomp_notify_send(filter, buf); case SECCOMP_IOCTL_NOTIF_ID_VALID: return seccomp_notify_id_valid(filter, buf); - case SECCOMP_IOCTL_NOTIF_ADDFD: - return seccomp_notify_addfd(filter, buf); + } + + /* Extensible Argument ioctls */ +#define EA_IOCTL(cmd) ((cmd) & ~(IOC_INOUT | IOCSIZE_MASK)) + switch (EA_IOCTL(cmd)) { + case EA_IOCTL(SECCOMP_IOCTL_NOTIF_ADDFD): + return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd)); default: return -EINVAL; } +#undef EA_IOCTL } static __poll_t seccomp_notify_poll(struct file *file, diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 95b134933831..cf1480e498ea 100644 --- a/tools/testing/selftests/seccomp/seccomp_bpf.c +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c @@ -216,7 +216,6 @@ struct seccomp_notif_sizes { #define SECCOMP_ADDFD_FLAG_SETFD (1UL << 0) /* Specify remote fd */ struct seccomp_notif_addfd { - __u64 size; __u64 id; __u32 flags; __u32 srcfd; @@ -225,6 +224,22 @@ struct seccomp_notif_addfd { }; #endif +struct seccomp_notif_addfd_small { + __u64 id; + char weird[4]; +}; +#define SECCOMP_IOCTL_NOTIF_ADDFD_SMALL \ + SECCOMP_IOW(3, struct seccomp_notif_addfd_small) + +struct seccomp_notif_addfd_big { + union { + struct seccomp_notif_addfd addfd; + char buf[sizeof(struct seccomp_notif_addfd) + 8]; + }; +}; +#define SECCOMP_IOCTL_NOTIF_ADDFD_BIG \ + SECCOMP_IOWR(3, struct seccomp_notif_addfd_big) + #ifndef PTRACE_EVENTMSG_SYSCALL_ENTRY #define PTRACE_EVENTMSG_SYSCALL_ENTRY 1 #define PTRACE_EVENTMSG_SYSCALL_EXIT 2 @@ -3853,6 +3868,8 @@ TEST(user_notification_sendfd) long ret; int status, listener, memfd, fd; struct seccomp_notif_addfd addfd = {}; + struct seccomp_notif_addfd_small small = {}; + struct seccomp_notif_addfd_big big = {}; struct seccomp_notif req = {}; struct seccomp_notif_resp resp = {}; /* 100 ms */ @@ -3882,7 +3899,6 @@ TEST(user_notification_sendfd) ASSERT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_RECV, &req), 0); - addfd.size = sizeof(addfd); addfd.srcfd = memfd; addfd.newfd = 0; addfd.id = req.id; @@ -3906,6 +3922,16 @@ TEST(user_notification_sendfd) EXPECT_EQ(errno, EINVAL); addfd.newfd = 0; + /* Verify small size cannot be set */ + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_SMALL, &small), -1); + EXPECT_EQ(errno, EINVAL); + + /* Verify we can't send bits filled in unknown buffer area */ + memset(&big, 0xAA, sizeof(big)); + big.addfd = addfd; + EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_BIG, &big), -1); + EXPECT_EQ(errno, E2BIG); + /* Verify we can set an arbitrary remote fd */ fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd); /* @@ -3921,10 +3947,15 @@ TEST(user_notification_sendfd) EXPECT_EQ(ret, 0); } + /* Verify we can set an arbitrary remote fd with large size */ + memset(&big, 0x0, sizeof(big)); + big.addfd = addfd; + fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD_BIG, &big); + EXPECT_EQ(fd, 6); + /* Verify we can set a specific remote fd */ addfd.newfd = 42; addfd.flags = SECCOMP_ADDFD_FLAG_SETFD; - fd = ioctl(listener, SECCOMP_IOCTL_NOTIF_ADDFD, &addfd); EXPECT_EQ(fd, 42); ret = filecmp(getpid(), pid, memfd, fd); @@ -3935,10 +3966,10 @@ TEST(user_notification_sendfd) EXPECT_EQ(ret, 0); } + /* Resume syscall */ resp.id = req.id; resp.error = 0; resp.val = USER_NOTIF_MAGIC; - EXPECT_EQ(ioctl(listener, SECCOMP_IOCTL_NOTIF_SEND, &resp), 0); /* @@ -4006,7 +4037,6 @@ TEST(user_notification_sendfd_rlimit) ASSERT_EQ(prlimit(pid, RLIMIT_NOFILE, &lim, NULL), 0); - addfd.size = sizeof(addfd); addfd.srcfd = memfd; addfd.newfd_flags = O_CLOEXEC; addfd.newfd = 0;