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 <[email protected]>
Signed-off-by: Jiachen Zhang <[email protected]>
Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
---
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(),
+ };
+ err = ovl_create_or_link(new, inode, &attr, ovl_type_origin(old));
if (err)
iput(inode);
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 87759165d32b..85043123a103 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -655,6 +655,8 @@ struct ovl_cattr {
umode_t mode;
const char *link;
struct dentry *hardlink;
+ kuid_t fsuid;
+ kgid_t fsgid;
};
#define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
--
2.32.1 (Apple Git-133)
On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote:
> On Thu, 25 Aug 2022 at 15:08, Zhang Tianci
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Jiachen Zhang <[email protected]>
> > Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
> > ---
> > 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(). Plus, this is also what has been done before the
commit 51f7e52dc943 ("ovl: share inode for hard link") iiuc.
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().
On Thu, 25 Aug 2022 at 15:08, Zhang Tianci
<[email protected]> 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 <[email protected]>
> Signed-off-by: Jiachen Zhang <[email protected]>
> Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
> ---
> 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().
Thanks,
Miklos
On Wed, 31 Aug 2022 at 15:43, Christian Brauner <[email protected]> wrote:
>
> On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote:
> > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci
> > <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Jiachen Zhang <[email protected]>
> > > Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
> > > ---
> > > 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
On Wed, Aug 31, 2022 at 03:53:58PM +0200, Miklos Szeredi wrote:
> On Wed, 31 Aug 2022 at 15:43, Christian Brauner <[email protected]> wrote:
> >
> > On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote:
> > > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci
> > > <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Jiachen Zhang <[email protected]>
> > > > Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
> > > > ---
> > > > 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.
Yeah, I know what you mean but it's borderline in so far as the
underlying fs might still perform permission checking based on the
caller's fs{g,u}id which together with what I'm saying below in a bit
was what made me go oh well, we should use the caller's fs{g,u}id then
for consistency.
>
> > 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.
What I meant was back then even for link the fs{g,u}id was based on a
newly allocated inode->i_{g,u}id that was initialized through
inode_init_owner() with the caller's fs{g,u}id:
inode = ovl_new_inode(dentry->d_sb, mode);
if (!inode)
goto out;
err = ovl_copy_up(dentry->d_parent);
if (err)
goto out_iput;
inode_init_owner(inode, dentry->d_parent->d_inode, mode);
stat.mode = inode->i_mode;
old_cred = ovl_override_creds(dentry->d_sb);
err = -ENOMEM;
override_cred = prepare_creds();
if (override_cred) {
override_cred->fsuid = inode->i_uid;
override_cred->fsgid = inode->i_gid;
and it changed after that commit to be based on old inode. So emulating
the old behavior seemed the better approach.
>
> > 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.
Sounds good.
Christian Brauner <[email protected]> 于2022年8月31日周三 23:29写道：
>
> On Wed, Aug 31, 2022 at 03:53:58PM +0200, Miklos Szeredi wrote:
> > On Wed, 31 Aug 2022 at 15:43, Christian Brauner <[email protected]> wrote:
> > >
> > > On Wed, Aug 31, 2022 at 03:00:18PM +0200, Miklos Szeredi wrote:
> > > > On Thu, 25 Aug 2022 at 15:08, Zhang Tianci
> > > > <[email protected]> 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 <[email protected]>
> > > > > Signed-off-by: Jiachen Zhang <[email protected]>
> > > > > Signed-off-by: Christian Brauner (Microsoft) <[email protected]>
> > > > > ---
> > > > > 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.
>
> Yeah, I know what you mean but it's borderline in so far as the
> underlying fs might still perform permission checking based on the
> caller's fs{g,u}id which together with what I'm saying below in a bit
> was what made me go oh well, we should use the caller's fs{g,u}id then
> for consistency.
>
> >
> > > 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.
>
> What I meant was back then even for link the fs{g,u}id was based on a
> newly allocated inode->i_{g,u}id that was initialized through
> inode_init_owner() with the caller's fs{g,u}id:
>
> inode = ovl_new_inode(dentry->d_sb, mode);
> if (!inode)
> goto out;
>
> err = ovl_copy_up(dentry->d_parent);
> if (err)
> goto out_iput;
>
> inode_init_owner(inode, dentry->d_parent->d_inode, mode);
> stat.mode = inode->i_mode;
>
> old_cred = ovl_override_creds(dentry->d_sb);
> err = -ENOMEM;
> override_cred = prepare_creds();
> if (override_cred) {
> override_cred->fsuid = inode->i_uid;
> override_cred->fsgid = inode->i_gid;
>
> and it changed after that commit to be based on old inode. So emulating
> the old behavior seemed the better approach.
>
> >
> > > 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.
>
> Sounds good.
It looks like you've reached an agreement.
So I will send the v3 patch using the mounter's fs{g,u}id with the
proper comment in ovl_create_or_link().
Thanks,
Tianci