Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1423024 > unrolled thread

[PATCH] ovl: fix uid/gid when creating over whiteout

Back to article view | Back to linux.kernel

Contents

  [PATCH] ovl: fix uid/gid when creating over whiteout Miklos Szeredi <miklos@szeredi.hu> - 2016-06-15 15:40 +0200
    Re: [PATCH] ovl: fix uid/gid when creating over whiteout Miklos Szeredi <miklos@szeredi.hu> - 2016-06-15 16:10 +0200
      Re: [PATCH] ovl: fix uid/gid when creating over whiteout Vivek Goyal <vgoyal@redhat.com> - 2016-06-15 16:30 +0200
    Re: [PATCH] ovl: fix uid/gid when creating over whiteout Vivek Goyal <vgoyal@redhat.com> - 2016-06-15 16:10 +0200

#1423024 — [PATCH] ovl: fix uid/gid when creating over whiteout

Hi Vivek,

I've tested this to fix the regresion that Stephen reported.  I think this
also is a good base for the selinux fix.

Pushed to overlayfs-linus and overlayfs-next branches of

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git

Please let me know if you see any problem with this.

Thanks,
Miklos

---
From: Miklos Szeredi <mszeredi@redhat.com>
Subject: ovl: fix uid/gid when creating over whiteout

Fix a regression when creating a file over a whiteout.  The new
file/directory needs to use the current fsuid/fsgid, not the ones from the
mounter's credentials.

The refcounting is a bit tricky: prepare_creds() sets an original refcount,
override_creds() gets one more, which revert_cred() drops.  So

  1) we need to expicitly put the mounter's credentials when overriding
     with the updated one

  2) we need to put the original ref to the updated creds (and this can
     safely be done before revert_creds(), since we'll still have the ref
     from override_creds()).

Reported-by: Stephen Smalley <sds@tycho.nsa.gov>
Fixes: 3fe6e52f0626 ("ovl: override creds with the ones from the superblock mounter")
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/dir.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -405,12 +405,21 @@ static int ovl_create_or_link(struct den
 		err = ovl_create_upper(dentry, inode, &stat, link, hardlink);
 	} else {
 		const struct cred *old_cred;
+		struct cred *override_cred;
 
 		old_cred = ovl_override_creds(dentry->d_sb);
 
-		err = ovl_create_over_whiteout(dentry, inode, &stat, link,
-					       hardlink);
+		err = -ENOMEM;
+		override_cred = prepare_creds();
+		if (override_cred) {
+			override_cred->fsuid = old_cred->fsuid;
+			override_cred->fsgid = old_cred->fsgid;
+			put_cred(override_creds(override_cred));
+			put_cred(override_cred);
 
+			err = ovl_create_over_whiteout(dentry, inode, &stat,
+						       link, hardlink);
+		}
 		revert_creds(old_cred);
 	}
 

[toc] | [next] | [standalone]

#1423056

On Wed, Jun 15, 2016 at 4:01 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Wed, Jun 15, 2016 at 03:30:02PM +0200, Miklos Szeredi wrote:

>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -405,12 +405,21 @@ static int ovl_create_or_link(struct den
>>               err = ovl_create_upper(dentry, inode, &stat, link, hardlink);
>>       } else {
>>               const struct cred *old_cred;
>> +             struct cred *override_cred;
>>
>>               old_cred = ovl_override_creds(dentry->d_sb);
>>
>> -             err = ovl_create_over_whiteout(dentry, inode, &stat, link,
>> -                                            hardlink);
>> +             err = -ENOMEM;
>> +             override_cred = prepare_creds();
>> +             if (override_cred) {
>> +                     override_cred->fsuid = old_cred->fsuid;
>> +                     override_cred->fsgid = old_cred->fsgid;
>
> Hi Miklos,
>
> I am wondering if we are switching to tasks's ->fsuid and ->fsgid too
> early. ovl_create_over_whiteout() calls ovl_lookup_temp(workdir) and
> IIUC, task might not have permission to do lookup in workdir.
>
> Should we switch to this override_cred, just before ovl_create_real()
> so that task ->fsuid and ->fsgid are used only for creation purposes
> only.

For lookup in workdir only CAP_DAC_OVERRIDE should matter, the actual
value of the fsuid and fsgid should be irrelevant (user, group and
others all have zero permission on workdir).

Thanks,
Miklos

[toc] | [prev] | [next] | [standalone]

#1423077

On Wed, Jun 15, 2016 at 04:09:47PM +0200, Miklos Szeredi wrote:
> On Wed, Jun 15, 2016 at 4:01 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Wed, Jun 15, 2016 at 03:30:02PM +0200, Miklos Szeredi wrote:
> 
> >> --- a/fs/overlayfs/dir.c
> >> +++ b/fs/overlayfs/dir.c
> >> @@ -405,12 +405,21 @@ static int ovl_create_or_link(struct den
> >>               err = ovl_create_upper(dentry, inode, &stat, link, hardlink);
> >>       } else {
> >>               const struct cred *old_cred;
> >> +             struct cred *override_cred;
> >>
> >>               old_cred = ovl_override_creds(dentry->d_sb);
> >>
> >> -             err = ovl_create_over_whiteout(dentry, inode, &stat, link,
> >> -                                            hardlink);
> >> +             err = -ENOMEM;
> >> +             override_cred = prepare_creds();
> >> +             if (override_cred) {
> >> +                     override_cred->fsuid = old_cred->fsuid;
> >> +                     override_cred->fsgid = old_cred->fsgid;
> >
> > Hi Miklos,
> >
> > I am wondering if we are switching to tasks's ->fsuid and ->fsgid too
> > early. ovl_create_over_whiteout() calls ovl_lookup_temp(workdir) and
> > IIUC, task might not have permission to do lookup in workdir.
> >
> > Should we switch to this override_cred, just before ovl_create_real()
> > so that task ->fsuid and ->fsgid are used only for creation purposes
> > only.
> 
> For lookup in workdir only CAP_DAC_OVERRIDE should matter, the actual
> value of the fsuid and fsgid should be irrelevant (user, group and
> others all have zero permission on workdir).

Ok. Agreed that CAP_DAC_OVERRIDE should do.

Thanks
Vivek

[toc] | [prev] | [next] | [standalone]

#1423057

On Wed, Jun 15, 2016 at 03:30:02PM +0200, Miklos Szeredi wrote:
> Hi Vivek,
> 
> I've tested this to fix the regresion that Stephen reported.  I think this
> also is a good base for the selinux fix.
> 
> Pushed to overlayfs-linus and overlayfs-next branches of
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/vfs.git
> 
> Please let me know if you see any problem with this.
> 
> Thanks,
> Miklos
> 
> ---
> From: Miklos Szeredi <mszeredi@redhat.com>
> Subject: ovl: fix uid/gid when creating over whiteout
> 
> Fix a regression when creating a file over a whiteout.  The new
> file/directory needs to use the current fsuid/fsgid, not the ones from the
> mounter's credentials.
> 
> The refcounting is a bit tricky: prepare_creds() sets an original refcount,
> override_creds() gets one more, which revert_cred() drops.  So
> 
>   1) we need to expicitly put the mounter's credentials when overriding
>      with the updated one
> 
>   2) we need to put the original ref to the updated creds (and this can
>      safely be done before revert_creds(), since we'll still have the ref
>      from override_creds()).
> 
> Reported-by: Stephen Smalley <sds@tycho.nsa.gov>
> Fixes: 3fe6e52f0626 ("ovl: override creds with the ones from the superblock mounter")
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/dir.c |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -405,12 +405,21 @@ static int ovl_create_or_link(struct den
>  		err = ovl_create_upper(dentry, inode, &stat, link, hardlink);
>  	} else {
>  		const struct cred *old_cred;
> +		struct cred *override_cred;
>  
>  		old_cred = ovl_override_creds(dentry->d_sb);
>  
> -		err = ovl_create_over_whiteout(dentry, inode, &stat, link,
> -					       hardlink);
> +		err = -ENOMEM;
> +		override_cred = prepare_creds();
> +		if (override_cred) {
> +			override_cred->fsuid = old_cred->fsuid;
> +			override_cred->fsgid = old_cred->fsgid;

Hi Miklos,

I am wondering if we are switching to tasks's ->fsuid and ->fsgid too
early. ovl_create_over_whiteout() calls ovl_lookup_temp(workdir) and
IIUC, task might not have permission to do lookup in workdir.

Should we switch to this override_cred, just before ovl_create_real()
so that task ->fsuid and ->fsgid are used only for creation purposes
only.

Thanks
Vivek

> +			put_cred(override_creds(override_cred));
> +			put_cred(override_cred);
>  
> +			err = ovl_create_over_whiteout(dentry, inode, &stat,
> +						       link, hardlink);
> +		}
>  		revert_creds(old_cred);
>  	}
>  

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web