| Message ID | 20200723171227.446711-2-mic@digikod.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for O_MAYEXEC | expand |
Mickaël Salaün <mic@digikod.net> writes: > From: Kees Cook <keescook@chromium.org> > > Change uselib(2)' S_ISREG() error return to EACCES instead of EINVAL so > the behavior matches execve(2), and the seemingly documented value. > The "not a regular file" failure mode of execve(2) is explicitly > documented[1], but it is not mentioned in uselib(2)[2] which does, > however, say that open(2) and mmap(2) errors may apply. The documentation > for open(2) does not include a "not a regular file" error[3], but mmap(2) > does[4], and it is EACCES. Do you have enough visibility into uselib to be certain this will change will not cause regressions? My sense of uselib is that it would be easier to remove the system call entirely (I think it's last use was in libc5) than to validate that a change like this won't cause problems for the users of uselib. For the kernel what is important are real world users and the manpages are only important as far as they suggest what the real world users do. Eric > [1] http://man7.org/linux/man-pages/man2/execve.2.html#ERRORS > [2] http://man7.org/linux/man-pages/man2/uselib.2.html#ERRORS > [3] http://man7.org/linux/man-pages/man2/open.2.html#ERRORS > [4] http://man7.org/linux/man-pages/man2/mmap.2.html#ERRORS > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Signed-off-by: Kees Cook <keescook@chromium.org> > Acked-by: Christian Brauner <christian.brauner@ubuntu.com> > Link: https://lore.kernel.org/r/20200605160013.3954297-2-keescook@chromium.org > --- > fs/exec.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index e6e8a9a70327..d7c937044d10 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -141,11 +141,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > - error = -EINVAL; > + error = -EACCES; > if (!S_ISREG(file_inode(file)->i_mode)) > goto exit; > > - error = -EACCES; > if (path_noexec(&file->f_path)) > goto exit;
ebiederm@xmission.com (Eric W. Biederman) writes: > Mickaël Salaün <mic@digikod.net> writes: > >> From: Kees Cook <keescook@chromium.org> >> >> Change uselib(2)' S_ISREG() error return to EACCES instead of EINVAL so >> the behavior matches execve(2), and the seemingly documented value. >> The "not a regular file" failure mode of execve(2) is explicitly >> documented[1], but it is not mentioned in uselib(2)[2] which does, >> however, say that open(2) and mmap(2) errors may apply. The documentation >> for open(2) does not include a "not a regular file" error[3], but mmap(2) >> does[4], and it is EACCES. > > Do you have enough visibility into uselib to be certain this will change > will not cause regressions? > > My sense of uselib is that it would be easier to remove the system call > entirely (I think it's last use was in libc5) than to validate that a > change like this won't cause problems for the users of uselib. > > For the kernel what is important are real world users and the manpages > are only important as far as they suggest what the real world users > do. Hmm. My apologies. After reading the next patch I see that what really makes this safe is: 73601ea5b7b1 ("fs/open.c: allow opening only regular files during execve()"). As in practice this change has already been made and uselib simply can not reach the !S_ISREG test. It might make sense to drop this patch or include that reference in the next posting of this patch. Eric
diff --git a/fs/exec.c b/fs/exec.c index e6e8a9a70327..d7c937044d10 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -141,11 +141,10 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) if (IS_ERR(file)) goto out; - error = -EINVAL; + error = -EACCES; if (!S_ISREG(file_inode(file)->i_mode)) goto exit; - error = -EACCES; if (path_noexec(&file->f_path)) goto exit;