Bug 39143 – severe bug in sys_mkdir, vfs_mkdir is invoked twice

• Status: VERIFIED FIXED
• Product: Gentoo Linux

[Description jochen 2004-01-23 03:08:38 UTC]

The -r1 patch breaks sys_mkdir, it invokes vfs_mkdir twice. If creating a directory succeeds (with the first call of vfs_mkdir) the second returns error "File exists".

from xfs-sources-2.4.24-r1.patch.bz2:

-                       error = vfs_mkdir(nd.dentry->d_inode, dentry,
-                                         mode & ~current->fs->umask);
+                error = 0;
+
+                       if (!gr_acl_handle_mkdir(dentry, nd.dentry, nd.mnt))
+                               error = -EACCES;
+
+                       if(!error)
+                               error = vfs_mkdir(nd.dentry->d_inode, dentry,
+                                         mode & ~current->fs->umask);
+                       if(!error)
+                               gr_handle_create(dentry, nd.mnt);
+
+
+                       if (!IS_POSIXACL(nd.dentry->d_inode))
+                               mode &= ~current->fs->umask;
+                       error = vfs_mkdir(nd.dentry->d_inode, dentry, mode);

this second call ^^^^ should be removed.

Reproducible: Always
Steps to Reproduce:
1. mkdir foo
2.
3.

Actual Results:  
foo is created, error "Cannot create directory: File exists" is given

Expected Results:  
foo is created

[Comment 1 Brian Jackson (RETIRED) 2004-01-23 13:55:45 UTC]

actually I think it just needs some brackets

[Comment 2 jochen 2004-01-24 03:24:22 UTC]

hu?

This is the vanilla linux code:

                if (!IS_ERR(dentry)) {
                        error = vfs_mkdir(nd.dentry->d_inode, dentry,
                                          mode & ~current->fs->umask);
                        dput(dentry);
                }

This is the grsecurity code:

                if (!IS_ERR(dentry)) {
                        error = 0;

                        if (!gr_acl_handle_mkdir(dentry, nd.dentry, nd.mnt))
                                error = -EACCES;

                        if(!error)
                                error = vfs_mkdir(nd.dentry->d_inode, dentry,
                                          mode & ~current->fs->umask);
                        if(!error)
                                gr_handle_create(dentry, nd.mnt);
                        
                        dput(dentry);
                }

This is the POSIX ACL code:

                if (!IS_ERR(dentry)) {
                        if (!IS_POSIXACL(nd.dentry->d_inode))
                                mode &= ~current->fs->umask;
                        error = vfs_mkdir(nd.dentry->d_inode, dentry, mode);
                        dput(dentry);
                }

All only contain one call to vfs_mkdir, so the merged version should also only contain one.

In sys_mknod the POSIXACL is tested before all grsecurity calls. In open_namei POSIXACL is tested between the grsecurity calls and the actual vfs call. In sys_mkdir it is tested after the vfs_call...

Since in the grsecurity only version, "mode" is passed to the grsecurity functions "as is", I'd recommend doing the POSIXACL thing after the grsecurity calls but before the vfs call, like it is done in open_namei.

The merged code would be:

              if (!IS_ERR(dentry)) {
                        error = 0;

                        if (!gr_acl_handle_mkdir(dentry, nd.dentry, nd.mnt))
                                error = -EACCES;

                        if(!error) {
                                if (!IS_POSIXACL(nd.dentry->d_inode))
                                          mode &= ~current->fs->umask;
                                error = vfs_mkdir(nd.dentry->d_inode, dentry,
                                          mode);
                        }

                        if(!error)
                                gr_handle_create(dentry, nd.mnt);
                        
                        dput(dentry);
                }

similar changes should be made to sys_mknod

[Comment 3 Brian Jackson (RETIRED) 2004-01-24 06:05:12 UTC]

I was thinking at first glance that you could indent the last vfs_mkdir and use brackets to make that one code block, but your way seems correct (that's how gentoo-sources does it).

[Comment 4 Brian Jackson (RETIRED) 2004-02-14 08:17:10 UTC]

*** Bug 41462 has been marked as a duplicate of this bug. ***

[Comment 5 Jason Cox (RETIRED) 2004-04-08 22:28:22 UTC]

Closing. Bug appears fixed.

[Comment 6 Jason Cox (RETIRED) 2004-04-08 22:28:46 UTC]

Closed. Bug fixed.