* [PATCH 0/2] Fix S_ISDIR execve() errno @ 2020-08-13 23:17 Kees Cook 2020-08-13 23:17 ` [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() Kees Cook 2020-08-13 23:17 ` [PATCH 2/2] selftests/exec: Add file type errno tests Kees Cook 0 siblings, 2 replies; 6+ messages in thread From: Kees Cook @ 2020-08-13 23:17 UTC (permalink / raw) To: Andrew Morton Cc: Kees Cook, Marc Zyngier, Shuah Khan, Greg Kroah-Hartman, linux-kernel, linux-kselftest Hi Andrew, This fixes an errno change for execve() of directories, noticed by Marc Zyngier[1]. Along with the fix, include a regression test to avoid seeing this return in the future. Thanks! -Kees [1] https://lore.kernel.org/lkml/20200813151305.6191993b@why Kees Cook (2): exec: Restore EACCES of S_ISDIR execve() selftests/exec: Add file type errno tests fs/namei.c | 4 +- tools/testing/selftests/exec/.gitignore | 1 + tools/testing/selftests/exec/Makefile | 5 +- tools/testing/selftests/exec/non-regular.c | 196 +++++++++++++++++++++ 4 files changed, 203 insertions(+), 3 deletions(-) create mode 100755 tools/testing/selftests/exec/non-regular.c -- 2.25.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() 2020-08-13 23:17 [PATCH 0/2] Fix S_ISDIR execve() errno Kees Cook @ 2020-08-13 23:17 ` Kees Cook 2020-08-14 7:11 ` Greg Kroah-Hartman 2020-08-14 8:15 ` Marc Zyngier 2020-08-13 23:17 ` [PATCH 2/2] selftests/exec: Add file type errno tests Kees Cook 1 sibling, 2 replies; 6+ messages in thread From: Kees Cook @ 2020-08-13 23:17 UTC (permalink / raw) To: Andrew Morton Cc: Kees Cook, Marc Zyngier, Shuah Khan, Greg Kroah-Hartman, linux-kernel, linux-kselftest The return code for attempting to execute a directory has always been EACCES. Adjust the S_ISDIR exec test to reflect the old errno instead of the general EISDIR for other kinds of "open" attempts on directories. Reported-by: Marc Zyngier <maz@kernel.org> Link: https://lore.kernel.org/lkml/20200813151305.6191993b@why Fixes: 633fb6ac3980 ("exec: move S_ISREG() check earlier") Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/namei.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/namei.c b/fs/namei.c index 2112e578dccc..e99e2a9da0f7 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -2849,8 +2849,10 @@ static int may_open(const struct path *path, int acc_mode, int flag) case S_IFLNK: return -ELOOP; case S_IFDIR: - if (acc_mode & (MAY_WRITE | MAY_EXEC)) + if (acc_mode & MAY_WRITE) return -EISDIR; + if (acc_mode & MAY_EXEC) + return -EACCES; break; case S_IFBLK: case S_IFCHR: -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() 2020-08-13 23:17 ` [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() Kees Cook @ 2020-08-14 7:11 ` Greg Kroah-Hartman 2020-08-14 8:13 ` Greg Kroah-Hartman 2020-08-14 8:15 ` Marc Zyngier 1 sibling, 1 reply; 6+ messages in thread From: Greg Kroah-Hartman @ 2020-08-14 7:11 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Marc Zyngier, Shuah Khan, linux-kernel, linux-kselftest On Thu, Aug 13, 2020 at 04:17:22PM -0700, Kees Cook wrote: > The return code for attempting to execute a directory has always been > EACCES. Adjust the S_ISDIR exec test to reflect the old errno instead > of the general EISDIR for other kinds of "open" attempts on directories. > > Reported-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/lkml/20200813151305.6191993b@why > Fixes: 633fb6ac3980 ("exec: move S_ISREG() check earlier") > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/namei.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/namei.c b/fs/namei.c > index 2112e578dccc..e99e2a9da0f7 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2849,8 +2849,10 @@ static int may_open(const struct path *path, int acc_mode, int flag) > case S_IFLNK: > return -ELOOP; > case S_IFDIR: > - if (acc_mode & (MAY_WRITE | MAY_EXEC)) > + if (acc_mode & MAY_WRITE) > return -EISDIR; > + if (acc_mode & MAY_EXEC) > + return -EACCES; > break; > case S_IFBLK: > case S_IFCHR: Reviewed-by: Greg Kroah-Hartman <gregkh@google.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() 2020-08-14 7:11 ` Greg Kroah-Hartman @ 2020-08-14 8:13 ` Greg Kroah-Hartman 0 siblings, 0 replies; 6+ messages in thread From: Greg Kroah-Hartman @ 2020-08-14 8:13 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Marc Zyngier, Shuah Khan, linux-kernel, linux-kselftest On Fri, Aug 14, 2020 at 09:11:02AM +0200, Greg Kroah-Hartman wrote: > On Thu, Aug 13, 2020 at 04:17:22PM -0700, Kees Cook wrote: > > The return code for attempting to execute a directory has always been > > EACCES. Adjust the S_ISDIR exec test to reflect the old errno instead > > of the general EISDIR for other kinds of "open" attempts on directories. > > > > Reported-by: Marc Zyngier <maz@kernel.org> > > Link: https://lore.kernel.org/lkml/20200813151305.6191993b@why > > Fixes: 633fb6ac3980 ("exec: move S_ISREG() check earlier") > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > fs/namei.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/fs/namei.c b/fs/namei.c > > index 2112e578dccc..e99e2a9da0f7 100644 > > --- a/fs/namei.c > > +++ b/fs/namei.c > > @@ -2849,8 +2849,10 @@ static int may_open(const struct path *path, int acc_mode, int flag) > > case S_IFLNK: > > return -ELOOP; > > case S_IFDIR: > > - if (acc_mode & (MAY_WRITE | MAY_EXEC)) > > + if (acc_mode & MAY_WRITE) > > return -EISDIR; > > + if (acc_mode & MAY_EXEC) > > + return -EACCES; > > break; > > case S_IFBLK: > > case S_IFCHR: > > > Reviewed-by: Greg Kroah-Hartman <gregkh@google.com> And to round out the "let's use a different email address for each response, to drive accounting tools crazy!" effort, you can also add: Tested-by: Greg Kroah-Hartman <gregkh@android.com> thanks, greg "I don't have enough different email addresses" k-h ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() 2020-08-13 23:17 ` [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() Kees Cook 2020-08-14 7:11 ` Greg Kroah-Hartman @ 2020-08-14 8:15 ` Marc Zyngier 1 sibling, 0 replies; 6+ messages in thread From: Marc Zyngier @ 2020-08-14 8:15 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Shuah Khan, Greg Kroah-Hartman, linux-kernel, linux-kselftest On 2020-08-14 00:17, Kees Cook wrote: > The return code for attempting to execute a directory has always been > EACCES. Adjust the S_ISDIR exec test to reflect the old errno instead > of the general EISDIR for other kinds of "open" attempts on > directories. > > Reported-by: Marc Zyngier <maz@kernel.org> > Link: https://lore.kernel.org/lkml/20200813151305.6191993b@why > Fixes: 633fb6ac3980 ("exec: move S_ISREG() check earlier") > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/namei.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/namei.c b/fs/namei.c > index 2112e578dccc..e99e2a9da0f7 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -2849,8 +2849,10 @@ static int may_open(const struct path *path, > int acc_mode, int flag) > case S_IFLNK: > return -ELOOP; > case S_IFDIR: > - if (acc_mode & (MAY_WRITE | MAY_EXEC)) > + if (acc_mode & MAY_WRITE) > return -EISDIR; > + if (acc_mode & MAY_EXEC) > + return -EACCES; > break; > case S_IFBLK: > case S_IFCHR: Reviewed-by: Marc Zyngier <maz@kernel.org> M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] selftests/exec: Add file type errno tests 2020-08-13 23:17 [PATCH 0/2] Fix S_ISDIR execve() errno Kees Cook 2020-08-13 23:17 ` [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() Kees Cook @ 2020-08-13 23:17 ` Kees Cook 1 sibling, 0 replies; 6+ messages in thread From: Kees Cook @ 2020-08-13 23:17 UTC (permalink / raw) To: Andrew Morton Cc: Kees Cook, Marc Zyngier, Shuah Khan, Greg Kroah-Hartman, linux-kernel, linux-kselftest Make sure execve() returns the expected errno values for non-regular files. Signed-off-by: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/exec/.gitignore | 1 + tools/testing/selftests/exec/Makefile | 5 +- tools/testing/selftests/exec/non-regular.c | 196 +++++++++++++++++++++ 3 files changed, 200 insertions(+), 2 deletions(-) create mode 100755 tools/testing/selftests/exec/non-regular.c diff --git a/tools/testing/selftests/exec/.gitignore b/tools/testing/selftests/exec/.gitignore index 94b02a18f230..344a99c6da1b 100644 --- a/tools/testing/selftests/exec/.gitignore +++ b/tools/testing/selftests/exec/.gitignore @@ -10,3 +10,4 @@ execveat.denatured /recursion-depth xxxxxxxx* pipe +S_I*.test diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile index 4453b8f8def3..0a13b110c1e6 100644 --- a/tools/testing/selftests/exec/Makefile +++ b/tools/testing/selftests/exec/Makefile @@ -3,7 +3,7 @@ CFLAGS = -Wall CFLAGS += -Wno-nonnull CFLAGS += -D_GNU_SOURCE -TEST_PROGS := binfmt_script +TEST_PROGS := binfmt_script non-regular TEST_GEN_PROGS := execveat TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir pipe # Makefile is a run-time dependency, since it's accessed by the execveat test @@ -11,7 +11,8 @@ TEST_FILES := Makefile TEST_GEN_PROGS += recursion-depth -EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx* +EXTRA_CLEAN := $(OUTPUT)/subdir.moved $(OUTPUT)/execveat.moved $(OUTPUT)/xxxxx* \ + $(OUTPUT)/S_I*.test include ../lib.mk diff --git a/tools/testing/selftests/exec/non-regular.c b/tools/testing/selftests/exec/non-regular.c new file mode 100755 index 000000000000..cd3a34aca93e --- /dev/null +++ b/tools/testing/selftests/exec/non-regular.c @@ -0,0 +1,196 @@ +// SPDX-License-Identifier: GPL-2.0+ +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <unistd.h> +#include <sys/socket.h> +#include <sys/stat.h> +#include <sys/sysmacros.h> +#include <sys/types.h> + +#include "../kselftest_harness.h" + +/* Remove a file, ignoring the result if it didn't exist. */ +void rm(struct __test_metadata *_metadata, const char *pathname, + int is_dir) +{ + int rc; + + if (is_dir) + rc = rmdir(pathname); + else + rc = unlink(pathname); + + if (rc < 0) { + ASSERT_EQ(errno, ENOENT) { + TH_LOG("Not ENOENT: %s", pathname); + } + } else { + ASSERT_EQ(rc, 0) { + TH_LOG("Failed to remove: %s", pathname); + } + } +} + +FIXTURE(file) { + char *pathname; + int is_dir; +}; + +FIXTURE_VARIANT(file) +{ + const char *name; + int expected; + int is_dir; + void (*setup)(struct __test_metadata *_metadata, + FIXTURE_DATA(file) *self, + const FIXTURE_VARIANT(file) *variant); + int major, minor, mode; /* for mknod() */ +}; + +void setup_link(struct __test_metadata *_metadata, + FIXTURE_DATA(file) *self, + const FIXTURE_VARIANT(file) *variant) +{ + const char * const paths[] = { + "/bin/true", + "/usr/bin/true", + }; + int i; + + for (i = 0; i < ARRAY_SIZE(paths); i++) { + if (access(paths[i], X_OK) == 0) { + ASSERT_EQ(symlink(paths[i], self->pathname), 0); + return; + } + } + ASSERT_EQ(1, 0) { + TH_LOG("Could not find viable 'true' binary"); + } +} + +FIXTURE_VARIANT_ADD(file, S_IFLNK) +{ + .name = "S_IFLNK", + .expected = ELOOP, + .setup = setup_link, +}; + +void setup_dir(struct __test_metadata *_metadata, + FIXTURE_DATA(file) *self, + const FIXTURE_VARIANT(file) *variant) +{ + ASSERT_EQ(mkdir(self->pathname, 0755), 0); +} + +FIXTURE_VARIANT_ADD(file, S_IFDIR) +{ + .name = "S_IFDIR", + .is_dir = 1, + .expected = EACCES, + .setup = setup_dir, +}; + +void setup_node(struct __test_metadata *_metadata, + FIXTURE_DATA(file) *self, + const FIXTURE_VARIANT(file) *variant) +{ + dev_t dev; + int rc; + + dev = makedev(variant->major, variant->minor); + rc = mknod(self->pathname, 0755 | variant->mode, dev); + ASSERT_EQ(rc, 0) { + if (errno == EPERM) + SKIP(return, "Please run as root; cannot mknod(%s)", + variant->name); + } +} + +FIXTURE_VARIANT_ADD(file, S_IFBLK) +{ + .name = "S_IFBLK", + .expected = EACCES, + .setup = setup_node, + /* /dev/loop0 */ + .major = 7, + .minor = 0, + .mode = S_IFBLK, +}; + +FIXTURE_VARIANT_ADD(file, S_IFCHR) +{ + .name = "S_IFCHR", + .expected = EACCES, + .setup = setup_node, + /* /dev/zero */ + .major = 1, + .minor = 5, + .mode = S_IFCHR, +}; + +void setup_fifo(struct __test_metadata *_metadata, + FIXTURE_DATA(file) *self, + const FIXTURE_VARIANT(file) *variant) +{ + ASSERT_EQ(mkfifo(self->pathname, 0755), 0); +} + +FIXTURE_VARIANT_ADD(file, S_IFIFO) +{ + .name = "S_IFIFO", + .expected = EACCES, + .setup = setup_fifo, +}; + +FIXTURE_SETUP(file) +{ + ASSERT_GT(asprintf(&self->pathname, "%s.test", variant->name), 6); + self->is_dir = variant->is_dir; + + rm(_metadata, self->pathname, variant->is_dir); + variant->setup(_metadata, self, variant); +} + +FIXTURE_TEARDOWN(file) +{ + rm(_metadata, self->pathname, self->is_dir); +} + +TEST_F(file, exec_errno) +{ + char * const argv[2] = { (char * const)self->pathname, NULL }; + + EXPECT_LT(execv(argv[0], argv), 0); + EXPECT_EQ(errno, variant->expected); +} + +/* S_IFSOCK */ +FIXTURE(sock) +{ + int fd; +}; + +FIXTURE_SETUP(sock) +{ + self->fd = socket(AF_INET, SOCK_STREAM, 0); + ASSERT_GE(self->fd, 0); +} + +FIXTURE_TEARDOWN(sock) +{ + if (self->fd >= 0) + ASSERT_EQ(close(self->fd), 0); +} + +TEST_F(sock, exec_errno) +{ + char * const argv[2] = { " magic socket ", NULL }; + char * const envp[1] = { NULL }; + + EXPECT_LT(fexecve(self->fd, argv, envp), 0); + EXPECT_EQ(errno, EACCES); +} + +TEST_HARNESS_MAIN -- 2.25.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-14 8:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-13 23:17 [PATCH 0/2] Fix S_ISDIR execve() errno Kees Cook 2020-08-13 23:17 ` [PATCH 1/2] exec: Restore EACCES of S_ISDIR execve() Kees Cook 2020-08-14 7:11 ` Greg Kroah-Hartman 2020-08-14 8:13 ` Greg Kroah-Hartman 2020-08-14 8:15 ` Marc Zyngier 2020-08-13 23:17 ` [PATCH 2/2] selftests/exec: Add file type errno tests Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).