On Fri, Apr 24, 2020 at 12:07:15AM +0100, Ben Hutchings wrote: > 3.16.83-rc1 review patch. If anyone has any objections, please let me know. I do. This patch is currently known-buggy, see this thread: https://www.openwall.com/lists/oss-security/2020/01/28/2 It is (partially) fixed with these newer commits in 5.5 and 5.5.2: commit d0cb50185ae942b03c4327be322055d622dc79f6 Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx> Date: Sun Jan 26 09:29:34 2020 -0500 do_last(): fetch directory ->i_mode and ->i_uid before it's too late may_create_in_sticky() call is done when we already have dropped the reference to dir. Fixes: 30aba6656f61e (namei: allow restricted O_CREAT of FIFOs and regular files) Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx> commit d76341d93dedbcf6ed5a08dfc8bce82d3e9a772b Author: Al Viro <viro@xxxxxxxxxxxxxxxxxx> Date: Sat Feb 1 16:26:45 2020 +0000 vfs: fix do_last() regression commit 6404674acd596de41fd3ad5f267b4525494a891a upstream. Brown paperbag time: fetching ->i_uid/->i_mode really should've been done from nd->inode. I even suggested that, but the reason for that has slipped through the cracks and I went for dir->d_inode instead - made for more "obvious" patch. Analysis: - at the entry into do_last() and all the way to step_into(): dir (aka nd->path.dentry) is known not to have been freed; so's nd->inode and it's equal to dir->d_inode unless we are already doomed to -ECHILD. inode of the file to get opened is not known. - after step_into(): inode of the file to get opened is known; dir might be pointing to freed memory/be negative/etc. - at the call of may_create_in_sticky(): guaranteed to be out of RCU mode; inode of the file to get opened is known and pinned; dir might be garbage. The last was the reason for the original patch. Except that at the do_last() entry we can be in RCU mode and it is possible that nd->path.dentry->d_inode has already changed under us. In that case we are going to fail with -ECHILD, but we need to be careful; nd->inode is pointing to valid struct inode and it's the same as nd->path.dentry->d_inode in "won't fail with -ECHILD" case, so we should use that. Reported-by: "Rantala, Tommi T. (Nokia - FI/Espoo)" <tommi.t.rantala@xxxxxxxxx> Reported-by: syzbot+190005201ced78a74ad6@xxxxxxxxxxxxxxxxxxxxxxxxx Wearing-brown-paperbag: Al Viro <viro@xxxxxxxxxxxxxxxxxx> Cc: stable@xxxxxxxxxx Fixes: d0cb50185ae9 ("do_last(): fetch directory ->i_mode and ->i_uid before it's too late") Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> At least inclusion of the above fixes is mandatory for any backports. Also, I think no one has fixed the logic of may_create_in_sticky() so that it wouldn't unintentionally apply the "protection" when the file is neither a FIFO nor a regular file (something I found and mentioned in the oss-security posting above). > +/** > + * may_create_in_sticky - Check whether an O_CREAT open in a sticky directory > + * should be allowed, or not, on files that already > + * exist. > + * @dir: the sticky parent directory > + * @inode: the inode of the file to open > + * > + * Block an O_CREAT open of a FIFO (or a regular file) when: > + * - sysctl_protected_fifos (or sysctl_protected_regular) is enabled > + * - the file already exists > + * - we are in a sticky directory > + * - we don't own the file > + * - the owner of the directory doesn't own the file > + * - the directory is world writable > + * If the sysctl_protected_fifos (or sysctl_protected_regular) is set to 2 > + * the directory doesn't have to be world writable: being group writable will > + * be enough. > + * > + * Returns 0 if the open is allowed, -ve on error. > + */ > +static int may_create_in_sticky(struct dentry * const dir, > + struct inode * const inode) > +{ > + if ((!sysctl_protected_fifos && S_ISFIFO(inode->i_mode)) || > + (!sysctl_protected_regular && S_ISREG(inode->i_mode)) || > + likely(!(dir->d_inode->i_mode & S_ISVTX)) || > + uid_eq(inode->i_uid, dir->d_inode->i_uid) || > + uid_eq(current_fsuid(), inode->i_uid)) > + return 0; > + > + if (likely(dir->d_inode->i_mode & 0002) || > + (dir->d_inode->i_mode & 0020 && > + ((sysctl_protected_fifos >= 2 && S_ISFIFO(inode->i_mode)) || > + (sysctl_protected_regular >= 2 && S_ISREG(inode->i_mode))))) { > + return -EACCES; > + } > + return 0; > +} I think the implementation of may_create_in_sticky() should be rewritten such that it'd directly correspond to the textual description in the comment above. As we've seen, trying to write the code "more optimally" resulted in its logic actually being different from the description. Meanwhile, I think backporting known-so-buggy code is a bad idea. Alexander
