| Message ID | 20240307092603.16269-1-meted@linux.ibm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Petr Vorel |
| Headers | show |
| Series | [v1] fanotify14: fix anonymous pipe testcases | expand |
On Fri, Mar 8, 2024 at 2:43 PM Mete Durlu <meted@linux.ibm.com> wrote: > > When SElinux is configured (comes out of the box on most distros) and > is configured to enforcing (the default configuration), tests related > to anonymous pipes return EACCES instead of the expected errno EINVAL. > Fix the failures caused by the above condition by checking the SElinux > configuration and adjusting the errno accordingly. Hi Mete, Isn't the outcome of the test dependent on the SEpolicy rules? Not only if it is enforced? Sorry I have very little experience with SELinux. Thanks, Amir. > > Signed-off-by: Mete Durlu <meted@linux.ibm.com> > --- > testcases/kernel/syscalls/fanotify/fanotify14.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c > index d02d81495..61ed8c660 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]; > @@ -279,6 +294,8 @@ static void do_test(unsigned int number) > if (tc->pfd) { > dirfd = tc->pfd[0]; > path = NULL; > + if (is_selinux_enforcing()) > + tc->expected_errno = EACCES; > } > > tst_res(TINFO, "Testing %s with %s", > -- > 2.44.0 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On 3/8/24 14:39, Amir Goldstein wrote: > On Fri, Mar 8, 2024 at 2:43 PM Mete Durlu <meted@linux.ibm.com> wrote: >> >> When SElinux is configured (comes out of the box on most distros) and >> is configured to enforcing (the default configuration), tests related >> to anonymous pipes return EACCES instead of the expected errno EINVAL. >> Fix the failures caused by the above condition by checking the SElinux >> configuration and adjusting the errno accordingly. > > Hi Mete, > > Isn't the outcome of the test dependent on the SEpolicy rules? > Not only if it is enforced? > > Sorry I have very little experience with SELinux. > Hi Amir, I don't have SElinux experience either, on my proposed patch I only considered the default behavior but you are right different SElinux configurations may lead to different outcomes. I skimmed over SElinux wiki a little and now I think trying to verify the SElinux policy would be too cumbersome. Instead I propose two different solutions. 1. We can skip the anonymous pipe test cases when SElinux is in enforcing state. or 2. We can accept both EACESS and EINVAL as valid errnos when SElinux is in enforcing state. Personally option 2 sounds better to me since we would get more coverage that way. If either way sounds good I can send a v2 right away. How does that sound? Thank you. > >> >> Signed-off-by: Mete Durlu <meted@linux.ibm.com> >> --- >> testcases/kernel/syscalls/fanotify/fanotify14.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c >> index d02d81495..61ed8c660 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]; >> @@ -279,6 +294,8 @@ static void do_test(unsigned int number) >> if (tc->pfd) { >> dirfd = tc->pfd[0]; >> path = NULL; >> + if (is_selinux_enforcing()) >> + tc->expected_errno = EACCES; >> } >> >> tst_res(TINFO, "Testing %s with %s", >> -- >> 2.44.0 >> >> >> -- >> Mailing list info: https://lists.linux.it/listinfo/ltp
On Mon, Mar 11, 2024 at 4:53 PM Mete Durlu <meted@linux.ibm.com> wrote: > > On 3/8/24 14:39, Amir Goldstein wrote: > > On Fri, Mar 8, 2024 at 2:43 PM Mete Durlu <meted@linux.ibm.com> wrote: > >> > >> When SElinux is configured (comes out of the box on most distros) and > >> is configured to enforcing (the default configuration), tests related > >> to anonymous pipes return EACCES instead of the expected errno EINVAL. > >> Fix the failures caused by the above condition by checking the SElinux > >> configuration and adjusting the errno accordingly. > > > > Hi Mete, > > > > Isn't the outcome of the test dependent on the SEpolicy rules? > > Not only if it is enforced? > > > > Sorry I have very little experience with SELinux. > > > > Hi Amir, > > I don't have SElinux experience either, on my proposed patch I only > considered the default behavior but you are right different SElinux > configurations may lead to different outcomes. I skimmed over SElinux > wiki a little and now I think trying to verify the SElinux policy would > be too cumbersome. Instead I propose two different solutions. > > 1. We can skip the anonymous pipe test cases when SElinux is in > enforcing state. > > or > > 2. We can accept both EACESS and EINVAL as valid errnos when SElinux is > in enforcing state. > > Personally option 2 sounds better to me since we would get more coverage > that way. If either way sounds good I can send a v2 right away. How does > that sound? option 2 sounds good to me. Thanks, Amir.
Hi Mete, Amir, Li, [ Cc Li who knows more about SELinux :) ] > On Mon, Mar 11, 2024 at 4:53 PM Mete Durlu <meted@linux.ibm.com> wrote: > > On 3/8/24 14:39, Amir Goldstein wrote: > > > On Fri, Mar 8, 2024 at 2:43 PM Mete Durlu <meted@linux.ibm.com> wrote: > > >> When SElinux is configured (comes out of the box on most distros) and > > >> is configured to enforcing (the default configuration), tests related > > >> to anonymous pipes return EACCES instead of the expected errno EINVAL. > > >> Fix the failures caused by the above condition by checking the SElinux > > >> configuration and adjusting the errno accordingly. > > > Hi Mete, > > > Isn't the outcome of the test dependent on the SEpolicy rules? > > > Not only if it is enforced? > > > Sorry I have very little experience with SELinux. > > Hi Amir, > > I don't have SElinux experience either, on my proposed patch I only > > considered the default behavior but you are right different SElinux > > configurations may lead to different outcomes. I skimmed over SElinux > > wiki a little and now I think trying to verify the SElinux policy would > > be too cumbersome. Instead I propose two different solutions. > > 1. We can skip the anonymous pipe test cases when SElinux is in > > enforcing state. > > or > > 2. We can accept both EACESS and EINVAL as valid errnos when SElinux is > > in enforcing state. > > Personally option 2 sounds better to me since we would get more coverage > > that way. If either way sounds good I can send a v2 right away. How does > > that sound? > option 2 sounds good to me. Yes, EACESS for enforced SELinux is what we want. Mete, thank you for handling this. I can confirm it's a problem on SELinux enforced. And I suppose the current code works, but we need some modifications (please let me know if you don't have time for v2): * Put tst_selinux_enforcing() function into LTP library: you need to create lib/tst_selinux.c and include/tst_selinux.c. For inspiration have look at lib/tst_lockdown.c vv include/tst_lockdown.h. The reason is obvious: sooner or later we will reuse this functionality. * use access(), print also TINFO (similarly to lib/tst_lockdown.c) * /sys/fs/selinux vs. /selinux, selinux=1 vs. security=selinux (/proc/cmdline) @Li: TL;DR: reading just /sys/fs/selinux/enforce LGTM, but please check I suppose we can rely on selinuxfs being mounted on /sys/fs/selinux: $ mount | grep -i selinux selinuxfs on /sys/fs/selinux type selinuxfs (rw,nosuid,noexec,relatime) Long time ago the directory was just /selinux (RHEL 5 or 6?), that's why it's still checked in shell API testcases/lib/tst_security.sh. These systems are quite old to run newest LTP, right? From d41415eb5edae [1] I see it was kernel 3.0 => way too old to consider. I guess we cannot rely on selinux=1 or security=selinux to detect enforce mode. There is SECURITY_SELINUX_BOOTPARAM, when disabled thus there is no selinux=1 variable in /proc/cmdline, thus we cannot rely on it (instead of using /sys/fs/selinux). Also, kernel < v5.1 had SECURITY_SELINUX_BOOTPARAM_VALUE (removed in be6ec88f41ba94 in v5.1 [2]), another reason not to rely on selinux in /proc/cmdline. NOTE: as I noted previously we have support for SELinux (and AppArmor) detection in shell API testcases/lib/tst_security.sh, we might later create simple C binary in testcases/lib/ which will call function you create in C API (similarly to testcases/lib/tst_lockdown_enabled.c), but we can ignore it now. Kind regards, Petr [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d41415eb5edae [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be6ec88f41ba94 > Thanks, > Amir.
On 3/12/24 14:10, Petr Vorel wrote: > Hi Mete, Amir, Li, > > [ Cc Li who knows more about SELinux :) ] > >> On Mon, Mar 11, 2024 at 4:53 PM Mete Durlu <meted@linux.ibm.com> wrote: > >>> On 3/8/24 14:39, Amir Goldstein wrote: >>>> On Fri, Mar 8, 2024 at 2:43 PM Mete Durlu <meted@linux.ibm.com> wrote: > >>>>> When SElinux is configured (comes out of the box on most distros) and >>>>> is configured to enforcing (the default configuration), tests related >>>>> to anonymous pipes return EACCES instead of the expected errno EINVAL. >>>>> Fix the failures caused by the above condition by checking the SElinux >>>>> configuration and adjusting the errno accordingly. > >>>> Hi Mete, > >>>> Isn't the outcome of the test dependent on the SEpolicy rules? >>>> Not only if it is enforced? > >>>> Sorry I have very little experience with SELinux. > > >>> Hi Amir, > >>> I don't have SElinux experience either, on my proposed patch I only >>> considered the default behavior but you are right different SElinux >>> configurations may lead to different outcomes. I skimmed over SElinux >>> wiki a little and now I think trying to verify the SElinux policy would >>> be too cumbersome. Instead I propose two different solutions. > >>> 1. We can skip the anonymous pipe test cases when SElinux is in >>> enforcing state. > >>> or > >>> 2. We can accept both EACESS and EINVAL as valid errnos when SElinux is >>> in enforcing state. > >>> Personally option 2 sounds better to me since we would get more coverage >>> that way. If either way sounds good I can send a v2 right away. How does >>> that sound? > >> option 2 sounds good to me. > > Yes, EACESS for enforced SELinux is what we want. > > Mete, thank you for handling this. I can confirm it's a problem on SELinux > enforced. And I suppose the current code works, but we need some modifications > (please let me know if you don't have time for v2): Hi, I was hoping to solve this with a quick/small fix but I guess there is more to do. > * Put tst_selinux_enforcing() function into LTP library: you need to create > lib/tst_selinux.c and include/tst_selinux.c. For inspiration have look at > lib/tst_lockdown.c vv include/tst_lockdown.h. The reason is obvious: sooner or > later we will reuse this functionality. If there is no rush for this I can add this in as a separate patch series, but I am not sure when I can start. If this is urgent then probably someone else should do it. > * use access(), print also TINFO (similarly to lib/tst_lockdown.c) > > * /sys/fs/selinux vs. /selinux, selinux=1 vs. security=selinux (/proc/cmdline) > @Li: TL;DR: reading just /sys/fs/selinux/enforce LGTM, but please check > > I suppose we can rely on selinuxfs being mounted on /sys/fs/selinux: > > $ mount | grep -i selinux > selinuxfs on /sys/fs/selinux type selinuxfs (rw,nosuid,noexec,relatime) > > Long time ago the directory was just /selinux (RHEL 5 or 6?), that's why it's > still checked in shell API testcases/lib/tst_security.sh. These systems are > quite old to run newest LTP, right? From d41415eb5edae [1] I see it was kernel > 3.0 => way too old to consider. > > I guess we cannot rely on selinux=1 or security=selinux to detect enforce mode. > There is SECURITY_SELINUX_BOOTPARAM, when disabled thus there is no selinux=1 > variable in /proc/cmdline, thus we cannot rely on it (instead of using > /sys/fs/selinux). > > Also, kernel < v5.1 had SECURITY_SELINUX_BOOTPARAM_VALUE (removed in > be6ec88f41ba94 in v5.1 [2]), another reason not to rely on selinux in > /proc/cmdline. > NOTE: as I noted previously we have support for SELinux (and AppArmor) detection > in shell API testcases/lib/tst_security.sh, we might later create simple C > binary in testcases/lib/ which will call function you create in C API (similarly > to testcases/lib/tst_lockdown_enabled.c), but we can ignore it now. > > Kind regards, > Petr > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d41415eb5edae > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=be6ec88f41ba94 > >> Thanks, >> Amir.
diff --git a/testcases/kernel/syscalls/fanotify/fanotify14.c b/testcases/kernel/syscalls/fanotify/fanotify14.c index d02d81495..61ed8c660 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]; @@ -279,6 +294,8 @@ static void do_test(unsigned int number) if (tc->pfd) { dirfd = tc->pfd[0]; path = NULL; + if (is_selinux_enforcing()) + tc->expected_errno = EACCES; } tst_res(TINFO, "Testing %s with %s",
When SElinux is configured (comes out of the box on most distros) and is configured to enforcing (the default configuration), tests related to anonymous pipes return EACCES instead of the expected errno EINVAL. Fix the failures caused by the above condition by checking the SElinux configuration and adjusting the errno accordingly. Signed-off-by: Mete Durlu <meted@linux.ibm.com> --- testcases/kernel/syscalls/fanotify/fanotify14.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)