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

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

[Miklos Szeredi]

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);
 	}
 

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

[Vivek Goyal]

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);
>  	}
>  

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

[Miklos Szeredi]

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

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

[Vivek Goyal]

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