| Message ID | 20240805131721.765484-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | exec: drop a racy path_noexec check | expand |
> To my reading that path_noexec is still there only for debug, not > because of any security need. I don't think it's there for debug. I think that WARN_ON_ONCE() is based on the assumption that the mount properties can't change. IOW, someone must've thought that somehow stable mount properties are guaranteed after may_open() irrespective of how the file was opened. And in that sense they thought they might actually catch a bug. But originally it did serve a purpose... > > To that end just I propose just whacking it. ... the full history (afaict) is that once upon a time noexec and whether it was a regular file were checked in (precurors to) inode_permission(). It then got moved into the callers. The callers also called may_open() directly afterwards. So the noexec and i_mode check preceeded the call to may_open() and thus to inode_permission(). Then may_open() got moved into the open helpers but the noexec and i_mode checks stayed behind. So the order was now reversed. That in turn meant it was possible to see non-regular file exec requests in security_inode_permission(). So the order was restored by moving that check into may_open(). At that time it would've made sense to also wipe the path_noexec() from there. But having it in there isn't wrong. In procfs permission/eligibility checks often are checked as close to the open as possible. Worst case it's something similar here. But it's certainly wrong to splat about it.
On Mon, Aug 05, 2024 at 05:35:35PM +0200, Christian Brauner wrote: > But having it in there isn't wrong. In procfs permission/eligibility > checks often are checked as close to the open as possible. Worst case > it's something similar here. But it's certainly wrong to splat about it. Right, please keep the redundant check, but we can downgrade it from a WARN. It's caught stuff in the past, so I'd like to retain it until we really do feel safe enough to let it go.
On Mon, Aug 05, 2024 at 05:35:35PM +0200, Christian Brauner wrote: > > To my reading that path_noexec is still there only for debug, not > > because of any security need. > > I don't think it's there for debug. I think that WARN_ON_ONCE() is based > on the assumption that the mount properties can't change. IOW, someone > must've thought that somehow stable mount properties are guaranteed > after may_open() irrespective of how the file was opened. And in that > sense they thought they might actually catch a bug. That would be a neat trick, seeing that there'd never been anything to prevent mount -o remount,exec while something is executed on the filesystem in question. > But having it in there isn't wrong. In procfs permission/eligibility > checks often are checked as close to the open as possible. Worst case > it's something similar here. But it's certainly wrong to splat about it. Bury it.
On Tue, Aug 06, 2024 at 12:38:04AM +0100, Al Viro wrote: > On Mon, Aug 05, 2024 at 05:35:35PM +0200, Christian Brauner wrote: > > > To my reading that path_noexec is still there only for debug, not > > > because of any security need. > > > > I don't think it's there for debug. I think that WARN_ON_ONCE() is based > > on the assumption that the mount properties can't change. IOW, someone > > must've thought that somehow stable mount properties are guaranteed > > after may_open() irrespective of how the file was opened. And in that > > sense they thought they might actually catch a bug. > > That would be a neat trick, seeing that there'd never been anything to > prevent mount -o remount,exec while something is executed on the noexec, obviously...
On Mon, 05 Aug 2024 15:17:21 +0200, Mateusz Guzik wrote: > Both i_mode and noexec checks wrapped in WARN_ON stem from an artifact > of the previous implementation. They used to legitimately check for the > condition, but that got moved up in two commits: > 633fb6ac3980 ("exec: move S_ISREG() check earlier") > 0fd338b2d2cd ("exec: move path_noexec() check earlier") > > Instead of being removed said checks are WARN_ON'ed instead, which > has some debug value > > [...] Applied to the vfs.misc branch of the vfs/vfs.git tree. Patches in the vfs.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.misc [1/1] exec: drop a racy path_noexec check https://git.kernel.org/vfs/vfs/c/d1968fae98da
diff --git a/fs/exec.c b/fs/exec.c index a126e3d1cacb..2938cbe38343 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -145,13 +145,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) goto out; /* - * may_open() has already checked for this, so it should be - * impossible to trip now. But we need to be extra cautious - * and check again at the very end too. + * Check do_open_execat() for an explanation. */ error = -EACCES; - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) + if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode))) goto exit; error = -ENOEXEC; @@ -954,7 +951,6 @@ EXPORT_SYMBOL(transfer_args_to_stack); static struct file *do_open_execat(int fd, struct filename *name, int flags) { struct file *file; - int err; struct open_flags open_exec_flags = { .open_flag = O_LARGEFILE | O_RDONLY | __FMODE_EXEC, .acc_mode = MAY_EXEC, @@ -971,24 +967,21 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) file = do_filp_open(fd, name, &open_exec_flags); if (IS_ERR(file)) - goto out; + return file; /* - * may_open() has already checked for this, so it should be - * impossible to trip now. But we need to be extra cautious - * and check again at the very end too. + * Validate the type. + * + * In the past the regular type check was here. It moved to may_open() in + * 633fb6ac3980 ("exec: move S_ISREG() check earlier"). Since then it is + * an invariant that all non-regular files error out before we get here. */ - err = -EACCES; - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) - goto exit; + if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode))) { + fput(file); + return ERR_PTR(-EACCES); + } -out: return file; - -exit: - fput(file); - return ERR_PTR(err); } /**
Both i_mode and noexec checks wrapped in WARN_ON stem from an artifact of the previous implementation. They used to legitimately check for the condition, but that got moved up in two commits: 633fb6ac3980 ("exec: move S_ISREG() check earlier") 0fd338b2d2cd ("exec: move path_noexec() check earlier") Instead of being removed said checks are WARN_ON'ed instead, which has some debug value However, the spurious path_noexec check is racy, resulting in unwarranted warnings should someone race with setting the noexec flag. One can note there is more to perm-checking whether execve is allowed and none of the conditions are guaranteed to still hold after they were tested for. Additionally this does not validate whether the code path did any perm checking to begin with -- it will pass if the inode happens to be regular. As such remove the racy check. The S_ISREG thing is kept for the time being since it does not hurt. Reword the commentary and do small tidy ups while here. Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- On Mon, Aug 05, 2024 at 11:26:19AM +0200, Christian Brauner wrote: > I think the immediate solution is to limit the scope of the > WARN_ON_ONCE() to the ->i_mode check. > To my reading that path_noexec is still there only for debug, not because of any security need. To that end just I propose just whacking it. fs/exec.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)