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=-2.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no 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 76FDBC742A5 for ; Fri, 12 Jul 2019 04:17:47 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 46DB720644 for ; Fri, 12 Jul 2019 04:17:47 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ih3PxJrC" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 46DB720644 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=zeniv.linux.org.uk Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=UYMiRrCHBBl8WlFftPUicpM55benoVilWwtZ6r1MY+M=; b=ih3PxJrCqUZLw7 uX+pWCVY675y2XZk0ztws3P8fcJgFli9Exqd8HbVkAg5cKWWIwZpT5OA7q95U8qiQqyXD1iCweq+L 4BZ8M5AAczuxG2HBcTIghNEvb5srH1ogYy5CAEY54xUtdLVLUlGM2M3b7GWXetkPj3PCQ30vAqIyf FUwvalB/qWIPdyEIbwDdYAPi15f4JmfFEzepDBNpPYg4ay4lcA0qcW+63UJo6VpKkopPM9e5aPK/s 3NSKmbMtUXbNudPo2tlnxVJZi2xlmQxqZuPQJJIgu3OzrFb7wwoi7CCHTuYEE7x+R74RI5b0tSVNk SpohRVWcn7p61tcNCRqQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hln0C-00024T-Lb; Fri, 12 Jul 2019 04:17:44 +0000 Received: from zeniv.linux.org.uk ([195.92.253.2]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hln08-0001xg-Bk for linux-arm-kernel@lists.infradead.org; Fri, 12 Jul 2019 04:17:42 +0000 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.92 #3 (Red Hat Linux)) id 1hlmxT-000330-0Q; Fri, 12 Jul 2019 04:14:55 +0000 Date: Fri, 12 Jul 2019 05:14:54 +0100 From: Al Viro To: Aleksa Sarai Subject: Re: [PATCH v9 01/10] namei: obey trailing magic-link DAC permissions Message-ID: <20190712041454.GG17978@ZenIV.linux.org.uk> References: <20190706145737.5299-1-cyphar@cyphar.com> <20190706145737.5299-2-cyphar@cyphar.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20190706145737.5299-2-cyphar@cyphar.com> User-Agent: Mutt/1.11.3 (2019-02-01) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190711_211740_404223_C2DD2207 X-CRM114-Status: GOOD ( 12.97 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org, Alexei Starovoitov , linux-kernel@vger.kernel.org, David Howells , linux-kselftest@vger.kernel.org, sparclinux@vger.kernel.org, Shuah Khan , linux-arch@vger.kernel.org, linux-s390@vger.kernel.org, Tycho Andersen , Aleksa Sarai , linux-arm-kernel@lists.infradead.org, linux-mips@vger.kernel.org, linux-xtensa@linux-xtensa.org, Kees Cook , Arnd Bergmann , Jann Horn , linuxppc-dev@lists.ozlabs.org, linux-m68k@lists.linux-m68k.org, Andy Lutomirski , Shuah Khan , David Drysdale , Christian Brauner , "J. Bruce Fields" , linux-parisc@vger.kernel.org, linux-api@vger.kernel.org, Chanho Min , Jeff Layton , Oleg Nesterov , Eric Biederman , linux-alpha@vger.kernel.org, linux-fsdevel@vger.kernel.org, Andrew Morton , Linus Torvalds , containers@lists.linux-foundation.org Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, Jul 07, 2019 at 12:57:28AM +1000, Aleksa Sarai wrote: > @@ -514,7 +516,14 @@ static void set_nameidata(struct nameidata *p, int dfd, struct filename *name) > p->stack = p->internal; > p->dfd = dfd; > p->name = name; > - p->total_link_count = old ? old->total_link_count : 0; > + p->total_link_count = 0; > + p->acc_mode = 0; > + p->opath_mask = FMODE_PATH_READ | FMODE_PATH_WRITE; > + if (old) { > + p->total_link_count = old->total_link_count; > + p->acc_mode = old->acc_mode; > + p->opath_mask = old->opath_mask; > + } Huh? Could somebody explain why traversals of NFS4 referrals should inherit ->acc_mode and ->opath_mask? > static __always_inline > -const char *get_link(struct nameidata *nd) > +const char *get_link(struct nameidata *nd, bool trailing) > { > struct saved *last = nd->stack + nd->depth - 1; > struct dentry *dentry = last->link.dentry; > @@ -1081,6 +1134,44 @@ const char *get_link(struct nameidata *nd) > } else { > res = get(dentry, inode, &last->done); > } > + /* If we just jumped it was because of a magic-link. */ > + if (unlikely(nd->flags & LOOKUP_JUMPED)) { That's not quite guaranteed (it is possible to bind a symlink on top of a regular file, and you will get LOOKUP_JUMPED on the entry into trailing_symlink() when looking the result up). Moreover, why bother with LOOKUP_JUMPED here? See that nd->last_type = LAST_BIND; several lines prior? That's precisely to be able to recognize those suckers. And _that_ would've avoided another piece of ugliness - your LOOKUP_JUMPED kludge forces you to handle that cra^Wsclero^Wvaluable security hardening in get_link(), instead of trailing_symlink() where you apparently want it to be. Simply because nd_jump_root() done later in get_link() will set LOOKUP_JUMPED for absolute symlinks, confusing your test. Moreover, I'm not sure that trailing_symlink() is the right place for that either - I would be rather tempted to fold do_o_path() into path_openat(), inline path_lookupat() there (as in s = path_init(nd, flags); while (!(error = link_path_walk(s, nd)) && ((error = lookup_last(nd)) > 0)) { s = trailing_symlink(nd); } if (!error) error = complete_walk(nd); if (!error && nd->flags & LOOKUP_DIRECTORY) if (!d_can_lookup(nd->path.dentry)) error = -ENOTDIR; if (!error) { audit_inode(nd->name, nd->path.dentry, 0); error = vfs_open(&nd->path, file); } terminate_walk(nd); - we don't need LOOKUP_DOWN there) and then we only care about the two callers of trailing_symlink() that are in path_openat(). Which is where you have your ->acc_mode and ->opath_mask without the need to dump them into nameidata. Or to bring that mess into the things like stat(2) et.al. - it simply doesn't belong there. In any case, this "bool trailing" is completely wrong; whether that check belongs in trailing_symlink() or (some of) its callers, putting it into get_link() is a mistake, forced by kludgy check for procfs-style symlinks. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel