| Message ID | 20240312120829.178305-2-meted@linux.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix fanotify anonymous pipe testcases | expand |
On Tue, Mar 12, 2024 at 2:09 PM Mete Durlu <meted@linux.ibm.com> wrote: > > When SElinux is in enforcing state and SEpolicies disallow anonymous > pipe usage with fanotify_mark(), related fanotify14 testcases fail with > EACCES instead of EINVAL. Accept both errnos when SElinux is in > enforcing state to correctly evaluate test results. > > Replace TST_EXP_FD_OR_FAIL with TST_EXP_FAIL when testing > fanotify_mark() as it returns -1 on failure and 0 on success not a file > descriptor. > > Signed-off-by: Mete Durlu <meted@linux.ibm.com> > --- > .../kernel/syscalls/fanotify/fanotify14.c | 32 +++++++++++++++++-- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c > index d02d81495..52c327dff 100644 > --- a/testcases/kernel/syscalls/fanotify/fanotify14.c > +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c > @@ -27,12 +27,14 @@ > #define _GNU_SOURCE > #include "tst_test.h" > #include <errno.h> > +#include <stdlib.h> > > #ifdef HAVE_SYS_FANOTIFY_H > #include "fanotify.h" > > #define MNTPOINT "mntpoint" > #define FILE1 MNTPOINT"/file1" > +#define SELINUX_STATUS_PATH "/sys/fs/selinux/enforce" > > /* > * List of inode events that are only available when notification group is > @@ -240,6 +242,19 @@ static struct test_case_t { > }, > }; > > +static int is_selinux_enforcing(void) > +{ > + char res; > + int fd; > + > + fd = open(SELINUX_STATUS_PATH, O_RDONLY); > + if (fd <= 0) > + return 0; > + SAFE_READ(1, fd, &res, 1); > + SAFE_CLOSE(fd); > + return atoi(&res); > +} > + > static void do_test(unsigned int number) > { > struct test_case_t *tc = &test_cases[number]; > @@ -275,17 +290,28 @@ static void do_test(unsigned int number) > /* Set mark on non-dir only when expecting error ENOTDIR */ > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > int dirfd = AT_FDCWD; > + int se_enforcing = 0; > > if (tc->pfd) { > dirfd = tc->pfd[0]; > path = NULL; > + se_enforcing = is_selinux_enforcing(); > } > > tst_res(TINFO, "Testing %s with %s", > tc->mark.desc, tc->mask.desc); > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > - tc->mask.flags, dirfd, path), > - tc->expected_errno); > + > + if (tc->pfd && se_enforcing) { > + const int exp_errs[] = {tc->expected_errno, EACCES}; > + > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > + tc->mask.flags, dirfd, path), > + exp_errs); > + } else { > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > + tc->mask.flags, dirfd, path), > + tc->expected_errno); > + } > This looks fine to me, but on second thought I am not sure how important it is to special case se_enforcing. We could probably always check for either error value. Let's see what Jan and Petr think. Thanks, Amir.
Hi all, ... > > static void do_test(unsigned int number) > > { > > struct test_case_t *tc = &test_cases[number]; > > @@ -275,17 +290,28 @@ static void do_test(unsigned int number) > > /* Set mark on non-dir only when expecting error ENOTDIR */ > > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > > int dirfd = AT_FDCWD; > > + int se_enforcing = 0; > > if (tc->pfd) { > > dirfd = tc->pfd[0]; > > path = NULL; > > + se_enforcing = is_selinux_enforcing(); nit: this check should be in the setup function to be done only once. (by default it's done twice, because we have 2 testcases with pfd, we support -iN parameter, thus it's actually 2*N.). I'll fix it before merge. > > } > > tst_res(TINFO, "Testing %s with %s", > > tc->mark.desc, tc->mask.desc); > > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > - tc->mask.flags, dirfd, path), > > - tc->expected_errno); > > + > > + if (tc->pfd && se_enforcing) { > > + const int exp_errs[] = {tc->expected_errno, EACCES}; > > + > > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + exp_errs); > > + } else { > > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > + tc->mask.flags, dirfd, path), > > + tc->expected_errno); > > + } > This looks fine to me, but on second thought I am not sure how important > it is to special case se_enforcing. > We could probably always check for either error value. I don't mind explicitly testing EACCES with SELinux. @Jan WDYT? With a diff below (I can change it before merge + I would do the following work to integrate this into the LTP C API): Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr diff --git testcases/kernel/syscalls/fanotify/fanotify14.c testcases/kernel/syscalls/fanotify/fanotify14.c index 52c327dff..89d59c8b2 100644 --- testcases/kernel/syscalls/fanotify/fanotify14.c +++ testcases/kernel/syscalls/fanotify/fanotify14.c @@ -49,6 +49,7 @@ static int pipes[2] = {-1, -1}; static int fanotify_fd; static int ignore_mark_unsupported; static int filesystem_mark_unsupported; +static int se_enforcing; static unsigned int supported_init_flags; struct test_case_flags_t { @@ -290,12 +291,10 @@ static void do_test(unsigned int number) /* Set mark on non-dir only when expecting error ENOTDIR */ const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; int dirfd = AT_FDCWD; - int se_enforcing = 0; if (tc->pfd) { dirfd = tc->pfd[0]; path = NULL; - se_enforcing = is_selinux_enforcing(); } tst_res(TINFO, "Testing %s with %s", @@ -360,6 +359,8 @@ static void do_setup(void) SAFE_FILE_PRINTF(FILE1, "0"); /* Create anonymous pipes to place marks on */ SAFE_PIPE2(pipes, O_CLOEXEC); + + se_enforcing = is_selinux_enforcing(); } static void do_cleanup(void) > Let's see what Jan and Petr think. > Thanks, > Amir.
On Wed 13-03-24 08:26:23, Petr Vorel wrote: > Hi all, > ... > > > > static void do_test(unsigned int number) > > > { > > > struct test_case_t *tc = &test_cases[number]; > > > @@ -275,17 +290,28 @@ static void do_test(unsigned int number) > > > /* Set mark on non-dir only when expecting error ENOTDIR */ > > > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > > > int dirfd = AT_FDCWD; > > > + int se_enforcing = 0; > > > > if (tc->pfd) { > > > dirfd = tc->pfd[0]; > > > path = NULL; > > > + se_enforcing = is_selinux_enforcing(); > nit: this check should be in the setup function to be done only once. > (by default it's done twice, because we have 2 testcases with pfd, we support > -iN parameter, thus it's actually 2*N.). I'll fix it before merge. > > > } > > > > tst_res(TINFO, "Testing %s with %s", > > > tc->mark.desc, tc->mask.desc); > > > - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > > - tc->mask.flags, dirfd, path), > > > - tc->expected_errno); > > > + > > > + if (tc->pfd && se_enforcing) { > > > + const int exp_errs[] = {tc->expected_errno, EACCES}; > > > + > > > + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > > + tc->mask.flags, dirfd, path), > > > + exp_errs); > > > + } else { > > > + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, > > > + tc->mask.flags, dirfd, path), > > > + tc->expected_errno); > > > + } > > > > This looks fine to me, but on second thought I am not sure how important > > it is to special case se_enforcing. > > We could probably always check for either error value. > > I don't mind explicitly testing EACCES with SELinux. @Jan WDYT? > > With a diff below (I can change it before merge + I would do the following work > to integrate this into the LTP C API): > Reviewed-by: Petr Vorel <pvorel@suse.cz> Yes, looks fine to me as well. I don't feel strongly whether we should accept EACCESS unconditionally or only with SELinux. I suspect eventually we might need to accept it unconditionally as there may be other security modules that would block addition of the mark. But let's see what the future brings. So feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza
On 3/13/24 08:26, Petr Vorel wrote: Hi, thanks for the review. I can send a v3 with the suggested changes if that will make things easier. Just let me know. >>> if (tc->pfd) { >>> dirfd = tc->pfd[0]; >>> path = NULL; >>> + se_enforcing = is_selinux_enforcing(); > nit: this check should be in the setup function to be done only once. > (by default it's done twice, because we have 2 testcases with pfd, we support > -iN parameter, thus it's actually 2*N.). I'll fix it before merge. >>> } > Nice catch! I fully forgot that there was a setup function while I was trying to find the best TST_ macro to use. >>> tst_res(TINFO, "Testing %s with %s", >>> tc->mark.desc, tc->mask.desc); >>> - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, >>> - tc->mask.flags, dirfd, path), >>> - tc->expected_errno); >>> + >>> + if (tc->pfd && se_enforcing) { >>> + const int exp_errs[] = {tc->expected_errno, EACCES}; >>> + >>> + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, >>> + tc->mask.flags, dirfd, path), >>> + exp_errs); >>> + } else { >>> + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, >>> + tc->mask.flags, dirfd, path), >>> + tc->expected_errno); >>> + } > > >> This looks fine to me, but on second thought I am not sure how important >> it is to special case se_enforcing. >> We could probably always check for either error value. > > I don't mind explicitly testing EACCES with SELinux. @Jan WDYT? > > With a diff below (I can change it before merge + I would do the following work > to integrate this into the LTP C API): > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Kind regards, > Petr > > diff --git testcases/kernel/syscalls/fanotify/fanotify14.c testcases/kernel/syscalls/fanotify/fanotify14.c > index 52c327dff..89d59c8b2 100644 > --- testcases/kernel/syscalls/fanotify/fanotify14.c > +++ testcases/kernel/syscalls/fanotify/fanotify14.c > @@ -49,6 +49,7 @@ static int pipes[2] = {-1, -1}; > static int fanotify_fd; > static int ignore_mark_unsupported; > static int filesystem_mark_unsupported; > +static int se_enforcing; > static unsigned int supported_init_flags; > > struct test_case_flags_t { > @@ -290,12 +291,10 @@ static void do_test(unsigned int number) > /* Set mark on non-dir only when expecting error ENOTDIR */ > const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; > int dirfd = AT_FDCWD; > - int se_enforcing = 0; > > if (tc->pfd) { > dirfd = tc->pfd[0]; > path = NULL; > - se_enforcing = is_selinux_enforcing(); > } > > tst_res(TINFO, "Testing %s with %s", > @@ -360,6 +359,8 @@ static void do_setup(void) > SAFE_FILE_PRINTF(FILE1, "0"); > /* Create anonymous pipes to place marks on */ > SAFE_PIPE2(pipes, O_CLOEXEC); > + > + se_enforcing = is_selinux_enforcing(); > } > > static void do_cleanup(void) >> Let's see what Jan and Petr think. > >> Thanks, >> Amir.
diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c index d02d81495..52c327dff 100644 --- a/testcases/kernel/syscalls/fanotify/fanotify14.c +++ b/testcases/kernel/syscalls/fanotify/fanotify14.c @@ -27,12 +27,14 @@ #define _GNU_SOURCE #include "tst_test.h" #include <errno.h> +#include <stdlib.h> #ifdef HAVE_SYS_FANOTIFY_H #include "fanotify.h" #define MNTPOINT "mntpoint" #define FILE1 MNTPOINT"/file1" +#define SELINUX_STATUS_PATH "/sys/fs/selinux/enforce" /* * List of inode events that are only available when notification group is @@ -240,6 +242,19 @@ static struct test_case_t { }, }; +static int is_selinux_enforcing(void) +{ + char res; + int fd; + + fd = open(SELINUX_STATUS_PATH, O_RDONLY); + if (fd <= 0) + return 0; + SAFE_READ(1, fd, &res, 1); + SAFE_CLOSE(fd); + return atoi(&res); +} + static void do_test(unsigned int number) { struct test_case_t *tc = &test_cases[number]; @@ -275,17 +290,28 @@ static void do_test(unsigned int number) /* Set mark on non-dir only when expecting error ENOTDIR */ const char *path = tc->expected_errno == ENOTDIR ? FILE1 : MNTPOINT; int dirfd = AT_FDCWD; + int se_enforcing = 0; if (tc->pfd) { dirfd = tc->pfd[0]; path = NULL; + se_enforcing = is_selinux_enforcing(); } tst_res(TINFO, "Testing %s with %s", tc->mark.desc, tc->mask.desc); - TST_EXP_FD_OR_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, - tc->mask.flags, dirfd, path), - tc->expected_errno); + + if (tc->pfd && se_enforcing) { + const int exp_errs[] = {tc->expected_errno, EACCES}; + + TST_EXP_FAIL_ARR(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, + tc->mask.flags, dirfd, path), + exp_errs); + } else { + TST_EXP_FAIL(fanotify_mark(fanotify_fd, FAN_MARK_ADD | tc->mark.flags, + tc->mask.flags, dirfd, path), + tc->expected_errno); + } /* * ENOTDIR are errors for events/flags not allowed on a non-dir inode.
When SElinux is in enforcing state and SEpolicies disallow anonymous pipe usage with fanotify_mark(), related fanotify14 testcases fail with EACCES instead of EINVAL. Accept both errnos when SElinux is in enforcing state to correctly evaluate test results. Replace TST_EXP_FD_OR_FAIL with TST_EXP_FAIL when testing fanotify_mark() as it returns -1 on failure and 0 on success not a file descriptor. Signed-off-by: Mete Durlu <meted@linux.ibm.com> --- .../kernel/syscalls/fanotify/fanotify14.c | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-)