Messages in this thread Patch in this message | | | Subject | [PATCH 2.6.5-mm4] sys_access race fix | From | Fabian Frederick <> | Date | Tue, 13 Apr 2004 20:43:01 +0200 |
| |
Andrew,
I'm trying to remove the race in sys_access code. AFAICS, fsuid is checked in "permission" level so I pushed real fsuid capture forward.At that state, I can task_lock (which was impossible before user_walk).Could you tell me if I can improve this one ?
Regards, Fabian diff -Naur orig/fs/open.c edited/fs/open.c --- orig/fs/open.c 2004-04-12 19:52:14.000000000 +0200 +++ edited/fs/open.c 2004-04-13 20:30:07.000000000 +0200 @@ -463,8 +463,8 @@ /* * access() needs to use the real uid/gid, not the effective uid/gid. - * We do this by temporarily clearing all FS-related capabilities and - * switching the fsuid/fsgid around to the real ones. + * We do this under task_lock by temporarily clearing all FS-related + * capabilities and switching the fsuid/fsgid around to the real ones. */ asmlinkage long sys_access(const char __user * filename, int mode) { @@ -476,40 +476,31 @@ if (mode & ~S_IRWXO) /* where's F_OK, X_OK, W_OK, R_OK? */ return -EINVAL; - old_fsuid = current->fsuid; - old_fsgid = current->fsgid; - old_cap = current->cap_effective; - - current->fsuid = current->uid; - current->fsgid = current->gid; - - /* - * Clear the capabilities if we switch to a non-root user - * - * FIXME: There is a race here against sys_capset. The - * capabilities can change yet we will restore the old - * value below. We should hold task_capabilities_lock, - * but we cannot because user_path_walk can sleep. - */ - if (current->uid) - cap_clear(current->cap_effective); - else - current->cap_effective = current->cap_permitted; - res = __user_walk(filename, LOOKUP_FOLLOW|LOOKUP_ACCESS, &nd); if (!res) { + task_lock(current); + old_fsuid = current->fsuid; + old_fsgid = current->fsgid; + old_cap = current->cap_effective; + current->fsuid = current->uid; + current->fsgid = current->gid; + + if (current->uid) + cap_clear(current->cap_effective); + else + current->cap_effective = current->cap_permitted; + res = permission(nd.dentry->d_inode, mode, &nd); /* SuS v2 requires we report a read only fs too */ if(!res && (mode & S_IWOTH) && IS_RDONLY(nd.dentry->d_inode) && !special_file(nd.dentry->d_inode->i_mode)) res = -EROFS; path_release(&nd); + current->fsuid = old_fsuid; + current->fsgid = old_fsgid; + current->cap_effective = old_cap; + task_unlock(current); } - - current->fsuid = old_fsuid; - current->fsgid = old_fsgid; - current->cap_effective = old_cap; - return res; } | |