Received: by 2002:ac0:a5a6:0:0:0:0:0 with SMTP id m35-v6csp23186imm; Thu, 27 Sep 2018 15:14:06 -0700 (PDT) X-Google-Smtp-Source: ACcGV63Z9pT9RYc5Xed99VB2Wc1HZORGwON30gPewT+SBKG2UGf0qO0hT45CdZzeloZbDEI0kv6u X-Received: by 2002:a17:902:368:: with SMTP id 95-v6mr12975188pld.305.1538086446597; Thu, 27 Sep 2018 15:14:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1538086446; cv=none; d=google.com; s=arc-20160816; b=hU6VpBSTGMVA7NtplaC06Peo9tKm4A4FgbPeDUs78yZcqjVlUwBFE6rILZMSiPIJW3 0s/ACe8koRjBXjBxaZVd4athOboc+s13z0xuro+hFuOqm8B99Sdhx5vGuW54PGzYT23h FD4ORWZOGvRRLVNRrL+QPXB++M28tI9/R3dPfPs8JVLPqALKfYxerSDNs0nbrWsgs9i7 jgLrAD3kP9L0jgZ6hBKj/Q+ksGR7ietcN9RGEIWjUw0b4EVj2L+5lE73WH/vHUTPxZ0l 47FnLMsPFcmbsvRe+gxGpNjEcVWDuKkwCOCsTO/51ogU5uhcuOm8coF6wFdRZRbIo+nP q3ZA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=Hs7jiu2zfPSrFO/1WCvowokq10I90tP/KPEeXv1dcas=; b=TDHwfH2mEp2o9fRbV0+r+Ad6BFTgMc5RTE8oVIeBPRba/mmgl5dSd/Np9iFJ5hgCmF om7tnwUXdXIrnaX3JSOU9EzA1K3uzKz6dp/08R6ukGs2h0FiQ/C5NO8okNe6qC5wdDv+ TmtMrtNl5J3Zn7qPF9VXxtkrjp/2a0dQf9dyfR+S9MWtgDhk1EkI5h7Tp5iN2KlWZNMv JUYN07WVDrJyKcn8XbIepewJ7WuuwTm2aKlmt2bbHCdPeLxk9vxtaeQp/BI9fESo0xbG VzSAD2hSzl39+Xvx8TRYjbROQamEvtnj0YH3S9SqKlDLZKx21HWJ1F5sytTiPoEYQI7C 5yXw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@tycho-ws.20150623.gappssmtp.com header.s=20150623 header.b=J0q8q7Ob; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id h125-v6si3193956pfg.138.2018.09.27.15.13.50; Thu, 27 Sep 2018 15:14:06 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@tycho-ws.20150623.gappssmtp.com header.s=20150623 header.b=J0q8q7Ob; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726362AbeI1EeI (ORCPT + 99 others); Fri, 28 Sep 2018 00:34:08 -0400 Received: from mail-oi1-f195.google.com ([209.85.167.195]:41325 "EHLO mail-oi1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725972AbeI1EeI (ORCPT ); Fri, 28 Sep 2018 00:34:08 -0400 Received: by mail-oi1-f195.google.com with SMTP id l197-v6so3594508oib.8 for ; Thu, 27 Sep 2018 15:13:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tycho-ws.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=Hs7jiu2zfPSrFO/1WCvowokq10I90tP/KPEeXv1dcas=; b=J0q8q7Obrao7GpiDfJ9L848YqRUs7VViDRCdiIXzTpyR2kh8WqdWQbRZWTnNH6kmvb bZwLGZFBcI5uGnVSjIDy26OxSwUyF2SI/EJFM7RjPKt8PXgyc2mdArzgPbDFpqQKAXc/ F9+SIGiS69ZCJmKhApAYW472g9T3X/99NomEL+JiFNBWKx9VaclJHotSJQae9CsJWWsU 85qOODNJdQM78oNUzyHet1IIMNqXQymollMQESX6ozrrXpZ85NUtTStE47bbY8a8f++N WnInghtJtA7MA7MLKSz3w28pZwLyxrGgUrzpA0Cws1YOTxo2j4e4TgtHjEWmGktbMULE MNtA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=Hs7jiu2zfPSrFO/1WCvowokq10I90tP/KPEeXv1dcas=; b=X71j/h0MqjDDKl6AYghfxurU6+dYxi+7wsAcKnz//kNvxhq20HIx+s/Zky2+aBYlVF tRo0AI0rxNIlPu6BqQyh7HsdvfkZwdGIll/ebzWJBXUPFpiAsRES8TdupNZ7XuJsd7MM GLZmzHd95WPijCQ/Peh8Ztz0BO55oo5QbhHm9/gmSCvDbtbJvxZWY/ouFC9CzBHAM/ys paMTqBkta2lHzMNQ0qzHB0Rlg3KP3ZBz1B8T0uyNf0sfJDrYy3onv4rwj1Q6Ax8kPnTf M3j0S23H4CK+/6f3JbymtGiNZC+ZiIEfuQLW4r1ESaUnjM2bvKedRjVJhX6zGBgdjKA8 In/A== X-Gm-Message-State: ABuFfogwAlg4EuQ66wma5gN9qNjte5Vtt3TmHESX12Ey5A2E6Yw0fZLI GhekZ5/v2IT5xZrE5hbfP+jfMQ== X-Received: by 2002:aca:b355:: with SMTP id c82-v6mr4523874oif.9.1538086420142; Thu, 27 Sep 2018 15:13:40 -0700 (PDT) Received: from cisco.cisco.com ([128.107.241.175]) by smtp.gmail.com with ESMTPSA id s98-v6sm1269023ota.47.2018.09.27.15.13.37 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 27 Sep 2018 15:13:39 -0700 (PDT) Date: Thu, 27 Sep 2018 16:13:36 -0600 From: Tycho Andersen To: Jann Horn Cc: Kees Cook , kernel list , containers@lists.linux-foundation.org, Linux API , Andy Lutomirski , Oleg Nesterov , "Eric W. Biederman" , "Serge E. Hallyn" , Christian Brauner , Tyler Hicks , suda.akihiro@lab.ntt.co.jp, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v7 5/6] seccomp: add a way to pass FDs via a notification fd Message-ID: <20180927221336.GC15491@cisco.cisco.com> References: <20180927151119.9989-1-tycho@tycho.ws> <20180927151119.9989-6-tycho@tycho.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 27, 2018 at 06:39:02PM +0200, Jann Horn wrote: > On Thu, Sep 27, 2018 at 5:11 PM Tycho Andersen wrote: > > This patch adds a way to insert FDs into the tracee's process (also > > close/overwrite fds for the tracee). This functionality is necessary to > > mock things like socketpair() or dup2() or similar, but since it depends on > > external (vfs) patches, I've left it as a separate patch as before so the > > core functionality can still be merged while we argue about this. Except > > this time it doesn't add any ugliness to the API :) > [...] > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > > index 17685803a2af..07a05ad59731 100644 > > --- a/kernel/seccomp.c > > +++ b/kernel/seccomp.c > > @@ -41,6 +41,8 @@ > > #include > > #include > > #include > > +#include > > +#include > > > > enum notify_state { > > SECCOMP_NOTIFY_INIT, > > @@ -1684,6 +1686,56 @@ static long seccomp_notify_id_valid(struct seccomp_filter *filter, > > return ret; > > } > > > > +static long seccomp_notify_put_fd(struct seccomp_filter *filter, > > + unsigned long arg) > > +{ > > + struct seccomp_notif_put_fd req; > > + void __user *buf = (void __user *)arg; > > + struct seccomp_knotif *knotif = NULL; > > + long ret; > > + > > + if (copy_from_user(&req, buf, sizeof(req))) > > + return -EFAULT; > > + > > + if (req.fd < 0 && req.to_replace < 0) > > + return -EINVAL; > > + > > + ret = mutex_lock_interruptible(&filter->notify_lock); > > + if (ret < 0) > > + return ret; > > + > > + ret = -ENOENT; > > + list_for_each_entry(knotif, &filter->notif->notifications, list) { > > + struct file *file = NULL; > > + > > + if (knotif->id != req.id) > > + continue; > > + > > + if (req.fd >= 0) > > + file = fget(req.fd); > > So here we take a reference on `file`. > > > + if (req.to_replace >= 0) { > > + ret = replace_fd_task(knotif->task, req.to_replace, > > + file, req.fd_flags); > > Then here we try to place the file in knotif->task's file descriptor > table. This can either fail (e.g. due to exceeded rlimit), in which > case nothing happens, or it can do do_dup2(), which first takes an > extra reference to the file, then places it in the task's fd table. > > Either way, afterwards, we still hold a reference to the file. > > > + } else { > > + unsigned long max_files; > > + > > + max_files = task_rlimit(knotif->task, RLIMIT_NOFILE); > > + ret = __alloc_fd(knotif->task->files, 0, max_files, > > + req.fd_flags); > > + if (ret < 0) > > + break; > > If we bail out here, we still hold a reference to `file`. > > Suggestion: Change this to "if (ret >= 0) {" and make the following > code conditional instead of breaking. > > > + __fd_install(knotif->task->files, ret, file); > > But if we reach this point, __fd_install() consumes the file pointer, > so `file` is a dangling pointer now. > > Suggestion: Add "break;" here. > > > + } > > Suggestion: Add "if (file != NULL) fput(file);" here. Ugh, yes, thanks. Tycho