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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4F15DECAAD4 for ; Wed, 31 Aug 2022 13:54:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231981AbiHaNyX (ORCPT ); Wed, 31 Aug 2022 09:54:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54560 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231737AbiHaNyM (ORCPT ); Wed, 31 Aug 2022 09:54:12 -0400 Received: from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com [IPv6:2a00:1450:4864:20::62d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B3845D59A3 for ; Wed, 31 Aug 2022 06:54:10 -0700 (PDT) Received: by mail-ej1-x62d.google.com with SMTP id h5so16951664ejb.3 for ; Wed, 31 Aug 2022 06:54:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=szeredi.hu; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=nvj3kJA7pYeTDCG4zfIRNhyv+Yp+WHJi5vOk9hX4ehQ=; b=UhBCe4jR0Eu4Pt32IhpDX9jLc5vFjXNjtGsi1hUJtsVNtiWrCcaVPX4MeGw3RNlsnz fnyQUctAeGJmJXTAtPs3al+O3Nfy0KLfaaFmaefhmucakkbaLhhj1iAQ4edWkO0d+mFG 5wyUlcHpUWt7+myqQPXUWSzo2Sqt5bTaTG6vk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=nvj3kJA7pYeTDCG4zfIRNhyv+Yp+WHJi5vOk9hX4ehQ=; b=6AArS5WvZaYVIgVkmzrajNYfTWfn0VSDEb9LkKQ2LFzhiKty/S/QigeU4x7Mp3HFKQ ltD3cbrhEEY4LhisQCNemensUu/T/jkt6HJpdUUwvV0vd+7ng2aKq3fV7VO2PK7sHNSM XkadMjtRtoBWgbzwJUD0kOkrAll/mJChJSH2o+KFSTFCPL/WpNsdRrAdEnqQrgFBl+QJ Y3mQNQ0Aq1igGInzAGvic41p3fN0rm5QrBsP6/OeHGYxn+2AOW6i7pOcYw9iSAoP8k6B S7KkVbq6fkTPPEO0sUrsJFiMKIzr4la3BOk7nc3E9uRV9V7iW5IH3tO+mmtaa2rhb0f9 4SBA== X-Gm-Message-State: ACgBeo3vEPFE6ibPwLOnFpHWNW2gl3A8hfokB9Czu3bthnx8jiJP7rdq JLgfExmUPRXNAQShVuTtBdUPiTPzCBZ6MsMMAD6KuQ== X-Google-Smtp-Source: AA6agR6j+JidZ74h6hhZse+1C10il+ZXBBnxSJifCfFJLsv+idXYxYOyWojsTFpw2ebUTAFOE9LXOcqYIu6SC0pP/ic= X-Received: by 2002:a17:907:16a0:b0:741:833b:c4c9 with SMTP id hc32-20020a17090716a000b00741833bc4c9mr10919165ejc.524.1661954049261; Wed, 31 Aug 2022 06:54:09 -0700 (PDT) MIME-Version: 1.0 References: <20220825130552.29587-1-zhangtianci.1997@bytedance.com> <20220831134327.dria7um4bhpk62mn@wittgenstein> In-Reply-To: <20220831134327.dria7um4bhpk62mn@wittgenstein> From: Miklos Szeredi Date: Wed, 31 Aug 2022 15:53:58 +0200 Message-ID: Subject: Re: [PATCH v2] ovl: Use current fsuid and fsgid in ovl_link() To: Christian Brauner Cc: Zhang Tianci , linux-unionfs@vger.kernel.org, linux-kernel@vger.kernel.org, amir73il@gmail.com, Jiachen Zhang Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-unionfs@vger.kernel.org On Wed, 31 Aug 2022 at 15:43, Christian Brauner wrote: > > On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote: > > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci > > wrote: > > > > > > There is a wrong case of link() on overlay: > > > $ mkdir /lower /fuse /merge > > > $ mount -t fuse /fuse > > > $ mkdir /fuse/upper /fuse/work > > > $ mount -t overlay /merge -o lowerdir=/lower,upperdir=/fuse/upper,\ > > > workdir=work > > > $ touch /merge/file > > > $ chown bin.bin /merge/file // the file's caller becomes "bin" > > > $ ln /merge/file /merge/lnkfile > > > > > > Then we will get an error(EACCES) because fuse daemon checks the link()'s > > > caller is "bin", it denied this request. > > > > > > In the changing history of ovl_link(), there are two key commits: > > > > > > The first is commit bb0d2b8ad296 ("ovl: fix sgid on directory") which > > > overrides the cred's fsuid/fsgid using the new inode. The new inode's > > > owner is initialized by inode_init_owner(), and inode->fsuid is > > > assigned to the current user. So the override fsuid becomes the > > > current user. We know link() is actually modifying the directory, so > > > the caller must have the MAY_WRITE permission on the directory. The > > > current caller may should have this permission. This is acceptable > > > to use the caller's fsuid. > > > > > > The second is commit 51f7e52dc943 ("ovl: share inode for hard link") > > > which removed the inode creation in ovl_link(). This commit move > > > inode_init_owner() into ovl_create_object(), so the ovl_link() just > > > give the old inode to ovl_create_or_link(). Then the override fsuid > > > becomes the old inode's fsuid, neither the caller nor the overlay's > > > creator! So this is incorrect. > > > > > > Fix this bug by using current fsuid/fsgid to do underlying fs's link(). > > > > > > Link: https://lore.kernel.org/all/20220817102951.xnvesg3a7rbv576x@wittgenstein/T > > > > > > Signed-off-by: Zhang Tianci > > > Signed-off-by: Jiachen Zhang > > > Signed-off-by: Christian Brauner (Microsoft) > > > --- > > > fs/overlayfs/dir.c | 16 +++++++++++----- > > > fs/overlayfs/overlayfs.h | 2 ++ > > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c > > > index 6b03457f72bb..dd84e6fc5f6e 100644 > > > --- a/fs/overlayfs/dir.c > > > +++ b/fs/overlayfs/dir.c > > > @@ -595,8 +595,8 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode, > > > err = -ENOMEM; > > > override_cred = prepare_creds(); > > > if (override_cred) { > > > - override_cred->fsuid = inode->i_uid; > > > - override_cred->fsgid = inode->i_gid; > > > + override_cred->fsuid = attr->fsuid; > > > + override_cred->fsgid = attr->fsgid; > > > if (!attr->hardlink) { > > > err = security_dentry_create_files_as(dentry, > > > attr->mode, &dentry->d_name, old_cred, > > > @@ -646,6 +646,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev, > > > inode_init_owner(&init_user_ns, inode, dentry->d_parent->d_inode, mode); > > > attr.mode = inode->i_mode; > > > > > > + attr.fsuid = inode->i_uid; > > > + attr.fsgid = inode->i_gid; > > > err = ovl_create_or_link(dentry, inode, &attr, false); > > > /* Did we end up using the preallocated inode? */ > > > if (inode != d_inode(dentry)) > > > @@ -702,6 +704,7 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > { > > > int err; > > > struct inode *inode; > > > + struct ovl_cattr attr; > > > > > > err = ovl_want_write(old); > > > if (err) > > > @@ -728,9 +731,12 @@ static int ovl_link(struct dentry *old, struct inode *newdir, > > > inode = d_inode(old); > > > ihold(inode); > > > > > > - err = ovl_create_or_link(new, inode, > > > - &(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)}, > > > - ovl_type_origin(old)); > > > + attr = (struct ovl_cattr) { > > > + .hardlink = ovl_dentry_upper(old), > > > + .fsuid = current_fsuid(), > > > + .fsgid = current_fsgid(), > > > + }; > > > > Why do we need to override fsuid/fsgid for the hardlink case? > > > > Wouldn't it be simpler to just use the mounter's creds unmodified in > > this case? The inode is not created in this case, so overriding with > > current uid/gid is not necessary, I think. > > > > Another way to look at it is: rename(A, B) is equivalent to an > > imaginary atomic [link(A, B) + unlink(A)] pair. But we don't override > > uid/gid for rename() or unlink() so why should we for link(). > > So my assumption has been that we want to override for any creation > request and so for the sake of consistency I would've expected to also > use that for ->link(). But link() is *not* a creation op. It's a namespace manipulation op. > Plus, this is also what has been done before the > commit 51f7e52dc943 ("ovl: share inode for hard link") iiuc. It wouldn't have mattered back then either, since the upper inode was linked and not copied. > Fwiw, I would've opted for consistency and even use the caller's > fs{g,u}id during ->rename() and ->unlink(). > > Right now the caller's fs{g,u}id - indirectly through inode_init_owner() > - is used to ensure that the ownership of newly created files in the > upper layer are based on the caller's not on the mounter's fs{g,u}id > afaict. If we continue to only override for those cases it would really > help that we document this in a good comment in ovl_create_or_link(). Yep. Thanks, Miklos