[v2] UBUNTU: SAUCE: shiftfs: prevent lower dentries from going negative during unlink

BugLink: https://bugs.launchpad.net/bugs/1860041

All non-special files (For shiftfs this only includes fifos and - for
this case - unix sockets - since we don't allow character and block
devices to be created.) go through shiftfs_open() and have their dentry
pinned through this codepath preventing it from going negative. But
fifos don't use the shiftfs fops but rather use the pipefifo_fops which
means they do not go through shiftfs_open() and thus don't have their
dentry pinned that way. Thus, the lower dentries for such files can go
negative on unlink causing segfaults. The following C program can be
used to reproduce the crash:

 #include <stdio.h>
 #include <fcntl.h>
 #include <unistd.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <unistd.h>
 #include <stdlib.h>

 int main(int argc, char *argv[])
 {
        struct stat stat;

        unlink("./bbb");

        int ret = mknod("./bbb", S_IFIFO|0666, 0);
        if (ret < 0)
                exit(1);

        int fd = open("./bbb", O_RDWR);
        if (fd < 0)
                exit(2);

        if (unlink("./bbb"))
                exit(4);

        fstat(fd, &stat);

        return 0;
 }

Similar to ecryptfs we need to dget() the lower dentry before calling
vfs_unlink() on it and dput() it afterwards.

Acked-by: Stefan Bader <stefan.bader@canonical.com>
Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
/* v1 */
Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html

/* v2 */
- Christian Brauner <christian.brauner@ubuntu.com>:
  - add missing ; after dget(lowerd)
---
 fs/shiftfs.c | 2 ++
 1 file changed, 2 insertions(+)


base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4

On 17.01.20 17:17, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860041
> 
> All non-special files (For shiftfs this only includes fifos and - for
> this case - unix sockets - since we don't allow character and block
> devices to be created.) go through shiftfs_open() and have their dentry
> pinned through this codepath preventing it from going negative. But
> fifos don't use the shiftfs fops but rather use the pipefifo_fops which
> means they do not go through shiftfs_open() and thus don't have their
> dentry pinned that way. Thus, the lower dentries for such files can go
> negative on unlink causing segfaults. The following C program can be
> used to reproduce the crash:
> 
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> 
>  int main(int argc, char *argv[])
>  {
>         struct stat stat;
> 
>         unlink("./bbb");
> 
>         int ret = mknod("./bbb", S_IFIFO|0666, 0);
>         if (ret < 0)
>                 exit(1);
> 
>         int fd = open("./bbb", O_RDWR);
>         if (fd < 0)
>                 exit(2);
> 
>         if (unlink("./bbb"))
>                 exit(4);
> 
>         fstat(fd, &stat);
> 
>         return 0;
>  }
> 
> Similar to ecryptfs we need to dget() the lower dentry before calling
> vfs_unlink() on it and dput() it afterwards.
> 
> Acked-by: Stefan Bader <stefan.bader@canonical.com>

If you send a V2, then its no longer acked and you should really send a
self-nack on the v1.

> Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> /* v1 */
> Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html
> 
> /* v2 */
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - add missing ; after dget(lowerd)
> ---
>  fs/shiftfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 04fba4689eb6..3623d02b061e 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -583,6 +583,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  	int err;
>  	const struct cred *oldcred;
>  
> +	dget(lowerd);
>  	oldcred = shiftfs_override_creds(dentry->d_sb);
>  	inode_lock_nested(loweri, I_MUTEX_PARENT);
>  	if (rmdir)
> @@ -602,6 +603,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  	inode_unlock(loweri);
>  
>  	shiftfs_copyattr(loweri, dir);
> +	dput(lowerd);
>  
>  	return err;
>  }
> 
> base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4
>

On 17.01.20 17:17, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860041
> 
> All non-special files (For shiftfs this only includes fifos and - for
> this case - unix sockets - since we don't allow character and block
> devices to be created.) go through shiftfs_open() and have their dentry
> pinned through this codepath preventing it from going negative. But
> fifos don't use the shiftfs fops but rather use the pipefifo_fops which
> means they do not go through shiftfs_open() and thus don't have their
> dentry pinned that way. Thus, the lower dentries for such files can go
> negative on unlink causing segfaults. The following C program can be
> used to reproduce the crash:
> 
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> 
>  int main(int argc, char *argv[])
>  {
>         struct stat stat;
> 
>         unlink("./bbb");
> 
>         int ret = mknod("./bbb", S_IFIFO|0666, 0);
>         if (ret < 0)
>                 exit(1);
> 
>         int fd = open("./bbb", O_RDWR);
>         if (fd < 0)
>                 exit(2);
> 
>         if (unlink("./bbb"))
>                 exit(4);
> 
>         fstat(fd, &stat);
> 
>         return 0;
>  }
> 
> Similar to ecryptfs we need to dget() the lower dentry before calling
> vfs_unlink() on it and dput() it afterwards.
> 
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> /* v1 */
> Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html
> 
> /* v2 */
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - add missing ; after dget(lowerd)

Agreed that I probably should have seen this but also test compiling before
submitting (even more so for simple things) does help (and that is from
self-experinece).


> ---
>  fs/shiftfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 04fba4689eb6..3623d02b061e 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -583,6 +583,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  	int err;
>  	const struct cred *oldcred;
>  
> +	dget(lowerd);
>  	oldcred = shiftfs_override_creds(dentry->d_sb);
>  	inode_lock_nested(loweri, I_MUTEX_PARENT);
>  	if (rmdir)
> @@ -602,6 +603,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  	inode_unlock(loweri);
>  
>  	shiftfs_copyattr(loweri, dir);
> +	dput(lowerd);
>  
>  	return err;
>  }
> 
> base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4
>

On Mon, Jan 20, 2020 at 04:08:10PM +0200, Stefan Bader wrote:
> On 17.01.20 17:17, Christian Brauner wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1860041
> > 
> > All non-special files (For shiftfs this only includes fifos and - for
> > this case - unix sockets - since we don't allow character and block
> > devices to be created.) go through shiftfs_open() and have their dentry
> > pinned through this codepath preventing it from going negative. But
> > fifos don't use the shiftfs fops but rather use the pipefifo_fops which
> > means they do not go through shiftfs_open() and thus don't have their
> > dentry pinned that way. Thus, the lower dentries for such files can go
> > negative on unlink causing segfaults. The following C program can be
> > used to reproduce the crash:
> > 
> >  #include <stdio.h>
> >  #include <fcntl.h>
> >  #include <unistd.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <unistd.h>
> >  #include <stdlib.h>
> > 
> >  int main(int argc, char *argv[])
> >  {
> >         struct stat stat;
> > 
> >         unlink("./bbb");
> > 
> >         int ret = mknod("./bbb", S_IFIFO|0666, 0);
> >         if (ret < 0)
> >                 exit(1);
> > 
> >         int fd = open("./bbb", O_RDWR);
> >         if (fd < 0)
> >                 exit(2);
> > 
> >         if (unlink("./bbb"))
> >                 exit(4);
> > 
> >         fstat(fd, &stat);
> > 
> >         return 0;
> >  }
> > 
> > Similar to ecryptfs we need to dget() the lower dentry before calling
> > vfs_unlink() on it and dput() it afterwards.
> > 
> > Acked-by: Stefan Bader <stefan.bader@canonical.com>
> > Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> > ---
> > /* v1 */
> > Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html
> > 
> > /* v2 */
> > - Christian Brauner <christian.brauner@ubuntu.com>:
> >   - add missing ; after dget(lowerd)
> 
> Agreed that I probably should have seen this but also test compiling before
> submitting (even more so for simple things) does help (and that is from
> self-experinece).

Yeah, I did but I had printk() statements in there and my intention was
to do a git fetch + git reset --hard but alas I forget the latter. So I
did test a correct change just not the final patch without the printks. :/

Christian

On Fri, Jan 17, 2020 at 04:17:06PM +0100, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860041
> 
> All non-special files (For shiftfs this only includes fifos and - for
> this case - unix sockets - since we don't allow character and block
> devices to be created.) go through shiftfs_open() and have their dentry
> pinned through this codepath preventing it from going negative. But
> fifos don't use the shiftfs fops but rather use the pipefifo_fops which
> means they do not go through shiftfs_open() and thus don't have their
> dentry pinned that way. Thus, the lower dentries for such files can go
> negative on unlink causing segfaults. The following C program can be
> used to reproduce the crash:
> 
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> 
>  int main(int argc, char *argv[])
>  {
>         struct stat stat;
> 
>         unlink("./bbb");
> 
>         int ret = mknod("./bbb", S_IFIFO|0666, 0);
>         if (ret < 0)
>                 exit(1);
> 
>         int fd = open("./bbb", O_RDWR);
>         if (fd < 0)
>                 exit(2);
> 
>         if (unlink("./bbb"))
>                 exit(4);
> 
>         fstat(fd, &stat);
> 
>         return 0;
>  }
> 
> Similar to ecryptfs we need to dget() the lower dentry before calling
> vfs_unlink() on it and dput() it afterwards.
> 
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

Applied to focal/master-next and unstable/master, thanks!

On Fri, Jan 17, 2020 at 04:17:06PM +0100, Christian Brauner wrote:
> BugLink: https://bugs.launchpad.net/bugs/1860041
> 
> All non-special files (For shiftfs this only includes fifos and - for
> this case - unix sockets - since we don't allow character and block
> devices to be created.) go through shiftfs_open() and have their dentry
> pinned through this codepath preventing it from going negative. But
> fifos don't use the shiftfs fops but rather use the pipefifo_fops which
> means they do not go through shiftfs_open() and thus don't have their
> dentry pinned that way. Thus, the lower dentries for such files can go
> negative on unlink causing segfaults. The following C program can be
> used to reproduce the crash:
> 
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <unistd.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
>  #include <stdlib.h>
> 
>  int main(int argc, char *argv[])
>  {
>         struct stat stat;
> 
>         unlink("./bbb");
> 
>         int ret = mknod("./bbb", S_IFIFO|0666, 0);
>         if (ret < 0)
>                 exit(1);
> 
>         int fd = open("./bbb", O_RDWR);
>         if (fd < 0)
>                 exit(2);
> 
>         if (unlink("./bbb"))
>                 exit(4);
> 
>         fstat(fd, &stat);
> 
>         return 0;
>  }
> 
> Similar to ecryptfs we need to dget() the lower dentry before calling
> vfs_unlink() on it and dput() it afterwards.
> 
> Acked-by: Stefan Bader <stefan.bader@canonical.com>
> Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
> /* v1 */
> Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html
> 
> /* v2 */
> - Christian Brauner <christian.brauner@ubuntu.com>:
>   - add missing ; after dget(lowerd)
> ---
>  fs/shiftfs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> index 04fba4689eb6..3623d02b061e 100644
> --- a/fs/shiftfs.c
> +++ b/fs/shiftfs.c
> @@ -583,6 +583,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  	int err;
>  	const struct cred *oldcred;
>  
> +	dget(lowerd);
>  	oldcred = shiftfs_override_creds(dentry->d_sb);
>  	inode_lock_nested(loweri, I_MUTEX_PARENT);
>  	if (rmdir)
> @@ -602,6 +603,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
>  	inode_unlock(loweri);
>  
>  	shiftfs_copyattr(loweri, dir);
> +	dput(lowerd);
>  
>  	return err;
>  }
> 
> base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4
> -- 
> 2.25.0
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

A late ack, but this looks fine.

Acked-by: Sultan Alsawaf <sultan.alsawaf@canonical.com>

On 2020-01-20 15:47:42 , Stefan Bader wrote:
> On 17.01.20 17:17, Christian Brauner wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1860041
> > 
> > All non-special files (For shiftfs this only includes fifos and - for
> > this case - unix sockets - since we don't allow character and block
> > devices to be created.) go through shiftfs_open() and have their dentry
> > pinned through this codepath preventing it from going negative. But
> > fifos don't use the shiftfs fops but rather use the pipefifo_fops which
> > means they do not go through shiftfs_open() and thus don't have their
> > dentry pinned that way. Thus, the lower dentries for such files can go
> > negative on unlink causing segfaults. The following C program can be
> > used to reproduce the crash:
> > 
> >  #include <stdio.h>
> >  #include <fcntl.h>
> >  #include <unistd.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> >  #include <unistd.h>
> >  #include <stdlib.h>
> > 
> >  int main(int argc, char *argv[])
> >  {
> >         struct stat stat;
> > 
> >         unlink("./bbb");
> > 
> >         int ret = mknod("./bbb", S_IFIFO|0666, 0);
> >         if (ret < 0)
> >                 exit(1);
> > 
> >         int fd = open("./bbb", O_RDWR);
> >         if (fd < 0)
> >                 exit(2);
> > 
> >         if (unlink("./bbb"))
> >                 exit(4);
> > 
> >         fstat(fd, &stat);
> > 
> >         return 0;
> >  }
> > 
> > Similar to ecryptfs we need to dget() the lower dentry before calling
> > vfs_unlink() on it and dput() it afterwards.
> > 
> > Acked-by: Stefan Bader <stefan.bader@canonical.com>
> 
> If you send a V2, then its no longer acked and you should really send a
> self-nack on the v1.
> 
> > Link: https://travis-ci.community/t/arm64-ppc64le-segfaults/6158/3
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> > ---
> > /* v1 */
> > Link: https://lists.ubuntu.com/archives/kernel-team/2020-January/106965.html
> > 
> > /* v2 */
> > - Christian Brauner <christian.brauner@ubuntu.com>:
> >   - add missing ; after dget(lowerd)
> > ---
> >  fs/shiftfs.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/shiftfs.c b/fs/shiftfs.c
> > index 04fba4689eb6..3623d02b061e 100644
> > --- a/fs/shiftfs.c
> > +++ b/fs/shiftfs.c
> > @@ -583,6 +583,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
> >  	int err;
> >  	const struct cred *oldcred;
> >  
> > +	dget(lowerd);
> >  	oldcred = shiftfs_override_creds(dentry->d_sb);
> >  	inode_lock_nested(loweri, I_MUTEX_PARENT);
> >  	if (rmdir)
> > @@ -602,6 +603,7 @@ static int shiftfs_rm(struct inode *dir, struct dentry *dentry, bool rmdir)
> >  	inode_unlock(loweri);
> >  
> >  	shiftfs_copyattr(loweri, dir);
> > +	dput(lowerd);
> >  
> >  	return err;
> >  }
> > 
> > base-commit: 5c6810aad94a79c86bee07cd19dba90b92f461b4
> > 
> 
> 




> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team