From: CAI Q. <ca...@cc...> - 2009-01-24 18:10:57
|
Hi, The following patch addes checking for SELinux. If it is enabled, the following entries are expected to be read successfully, /proc/self/attr/* /proc/self/task/[0-9]*/attr/* Otherwise, expecting read(2) return -1 with -EINVAL. It can be applied on the top of previously sent patch with the title, [PATCH] proc01: /proc/ppc64/rtas/error_log: read: Invalid argument Version 1 is broken. Signed-off-by: CAI Qian <ca...@cc...> --- testcases/kernel/fs/proc/proc01.c.p1 2009-01-24 19:08:51.843650731 +0800 +++ testcases/kernel/fs/proc/proc01.c 2009-01-25 02:06:00.001650743 +0800 @@ -25,6 +25,8 @@ * */ +#include "config.h" + #include <errno.h> /* for errno */ #include <stdio.h> /* for NULL */ #include <stdlib.h> /* for malloc() */ @@ -37,6 +39,10 @@ #include <fcntl.h> #include <fnmatch.h> +#ifdef HAVE_SELINUX_SELINUX_H +#include <selinux/selinux.h> +#endif + #include "test.h" #include "usctest.h" @@ -89,9 +95,23 @@ {"read", "/proc/self/mem", EIO}, {"read", "/proc/self/task/[0-9]*/mem", EIO}, {"read", "/proc/ppc64/rtas/error_log", EINVAL}, + {"read", "/proc/self/attr/*", EINVAL}, + {"read", "/proc/self/task/[0-9]*/attr/*", EINVAL}, {"", "", 0} }; +#ifdef HAVE_SELINUX_SELINUX_H +/* If SELinux is enabled, the following entries should be read + successfully. Note: SELinux libraries and headers should be installed + for the test to read those files. Otherwise, they will be skipped! */ +const char selinux_should_work[][PATH_MAX] = + { + "/proc/self/attr/*", + "/proc/self/task/[0-9]*/attr/*", + "" + }; +#endif + /* Known files that does not honor O_NONBLOCK, so they will hang the test while being read.*/ const char error_nonblock[][PATH_MAX] = @@ -105,6 +125,19 @@ { int i; +/* Should not see any error for certain entries if SELinux is enabled. */ +#ifdef HAVE_SELINUX_SELINUX_H + if (is_selinux_enabled()) + { + for (i = 0; selinux_should_work[i][0] != '\0'; i++) + { + if (!strcmp(obj, selinux_should_work[i]) + || !fnmatch(selinux_should_work[i], obj, FNM_PATHNAME)) + return 0; + } + } +#endif + for (i = 0; known_issues[i].err != 0; i++) if (tmperr == known_issues[i].err && (!strcmp(obj, known_issues[i].file) @@ -143,6 +176,16 @@ TEST_PAUSE; tst_tmpdir(); + +#ifdef HAVE_SELINUX_SELINUX_H + if (is_selinux_enabled()) + tst_resm(TINFO, "SELinux is enabled."); + else + tst_resm(TINFO, "SELinux is disabled."); +#else + tst_resm(TINFO, + "unable to determine if SELinux is disabled or not."); +#endif } void help() --- /dev/null 2009-01-24 15:26:18.326002642 +0800 +++ m4/ltp-selinux.m4 2009-01-24 19:56:54.660651164 +0800 @@ -0,0 +1,29 @@ +dnl +dnl Copyright (c) Red Hat Inc., 2009 +dnl +dnl This program is free software; you can redistribute it and/or +dnl modify it under the terms of the GNU General Public License as +dnl published by the Free Software Foundation; either version 2 of +dnl the License, or (at your option) any later version. +dnl +dnl This program is distributed in the hope that it will be useful, +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See +dnl the GNU General Public License for more details. +dnl +dnl You should have received a copy of the GNU General Public License +dnl along with this program; if not, write to the Free Software +dnl Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 +dnl USA + +dnl +dnl LTP_CHECK_SELINUX +dnl ---------------------------- +dnl +AC_DEFUN([LTP_CHECK_SELINUX], +[dnl +AC_CHECK_HEADERS(selinux/selinux.h,[ + SELINUX_LIBS="-lselinux"],[ + SELINUX_LIBS=""]) +AC_SUBST(SELINUX_LIBS) +]) --- testcases/kernel/fs/proc/Makefile.orig 2009-01-24 18:56:50.064650109 +0800 +++ testcases/kernel/fs/proc/Makefile 2009-01-25 02:00:24.316649805 +0800 @@ -16,12 +16,10 @@ # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA # -########################################################################### -# name of file : Makefile # -# description : make(1) description file for the send(2) tests. # -########################################################################### -CFLAGS+= -I../../../../include -LOADLIBES+= -L../../../../lib -lltp +include ../../../../config.mk + +CFLAGS+= -I../../../../include -Wall +LDLIBS+= -L../../../../lib -lltp $(SELINUX_LIBS) SRCS=$(wildcard *.c) TARGETS=$(patsubst %.c,%,$(SRCS)) @@ -33,5 +31,3 @@ clean: rm -f $(TARGETS) - - --- configure.ac.orig 2009-01-24 16:41:35.894653037 +0800 +++ configure.ac 2009-01-24 16:43:14.064654299 +0800 @@ -18,5 +18,6 @@ LTP_CHECK_SYSCALL_EVENTFD LTP_CHECK_SYSCALL_MODIFY_LDT LTP_CHECK_SYSCALL_SIGNALFD +LTP_CHECK_SELINUX AC_OUTPUT --- config.mk.in.orig 2009-01-24 19:01:43.472650122 +0800 +++ config.mk.in 2009-01-24 19:03:11.001651581 +0800 @@ -7,3 +7,4 @@ LDFLAGS = @LDFLAGS@ AIO_LIBS = @AIO_LIBS@ +SELINUX_LIBS = @SELINUX_LIBS@ |
From: Kamalesh B. <kam...@li...> - 2009-01-28 06:32:11
|
* CAI Qian <ca...@cc...> [2009-01-24 10:10:52]: > Hi, > > The following patch addes checking for SELinux. If it is enabled, the > following entries are expected to be read successfully, > > /proc/self/attr/* > /proc/self/task/[0-9]*/attr/* > > Otherwise, expecting read(2) return -1 with -EINVAL. > > It can be applied on the top of previously sent patch with the title, > > [PATCH] proc01: /proc/ppc64/rtas/error_log: read: Invalid argument > > Version 1 is broken. > > Signed-off-by: CAI Qian <ca...@cc...> > > --- testcases/kernel/fs/proc/proc01.c.p1 2009-01-24 19:08:51.843650731 +0800 > +++ testcases/kernel/fs/proc/proc01.c 2009-01-25 02:06:00.001650743 +0800 > @@ -25,6 +25,8 @@ > * > */ > > +#include "config.h" > + > #include <errno.h> /* for errno */ > #include <stdio.h> /* for NULL */ > #include <stdlib.h> /* for malloc() */ > @@ -37,6 +39,10 @@ > #include <fcntl.h> > #include <fnmatch.h> > > +#ifdef HAVE_SELINUX_SELINUX_H > +#include <selinux/selinux.h> > +#endif > + > #include "test.h" > #include "usctest.h" > > @@ -89,9 +95,23 @@ > {"read", "/proc/self/mem", EIO}, > {"read", "/proc/self/task/[0-9]*/mem", EIO}, > {"read", "/proc/ppc64/rtas/error_log", EINVAL}, > + {"read", "/proc/self/attr/*", EINVAL}, > + {"read", "/proc/self/task/[0-9]*/attr/*", EINVAL}, > {"", "", 0} > }; > > +#ifdef HAVE_SELINUX_SELINUX_H > +/* If SELinux is enabled, the following entries should be read > + successfully. Note: SELinux libraries and headers should be installed > + for the test to read those files. Otherwise, they will be skipped! */ > +const char selinux_should_work[][PATH_MAX] = > + { > + "/proc/self/attr/*", > + "/proc/self/task/[0-9]*/attr/*", > + "" > + }; > +#endif > + > /* Known files that does not honor O_NONBLOCK, so they will hang > the test while being read.*/ > const char error_nonblock[][PATH_MAX] = > @@ -105,6 +125,19 @@ > { > int i; > > +/* Should not see any error for certain entries if SELinux is enabled. */ > +#ifdef HAVE_SELINUX_SELINUX_H > + if (is_selinux_enabled()) > + { > + for (i = 0; selinux_should_work[i][0] != '\0'; i++) > + { > + if (!strcmp(obj, selinux_should_work[i]) > + || !fnmatch(selinux_should_work[i], obj, FNM_PATHNAME)) > + return 0; > + } > + } > +#endif > + > for (i = 0; known_issues[i].err != 0; i++) > if (tmperr == known_issues[i].err > && (!strcmp(obj, known_issues[i].file) > @@ -143,6 +176,16 @@ > TEST_PAUSE; > > tst_tmpdir(); > + > +#ifdef HAVE_SELINUX_SELINUX_H > + if (is_selinux_enabled()) > + tst_resm(TINFO, "SELinux is enabled."); > + else > + tst_resm(TINFO, "SELinux is disabled."); > +#else > + tst_resm(TINFO, > + "unable to determine if SELinux is disabled or not."); > +#endif > } > > void help() > > --- /dev/null 2009-01-24 15:26:18.326002642 +0800 > +++ m4/ltp-selinux.m4 2009-01-24 19:56:54.660651164 +0800 > @@ -0,0 +1,29 @@ > +dnl > +dnl Copyright (c) Red Hat Inc., 2009 > +dnl > +dnl This program is free software; you can redistribute it and/or > +dnl modify it under the terms of the GNU General Public License as > +dnl published by the Free Software Foundation; either version 2 of > +dnl the License, or (at your option) any later version. > +dnl > +dnl This program is distributed in the hope that it will be useful, > +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of > +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See > +dnl the GNU General Public License for more details. > +dnl > +dnl You should have received a copy of the GNU General Public License > +dnl along with this program; if not, write to the Free Software > +dnl Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 > +dnl USA > + > +dnl > +dnl LTP_CHECK_SELINUX > +dnl ---------------------------- > +dnl > +AC_DEFUN([LTP_CHECK_SELINUX], > +[dnl > +AC_CHECK_HEADERS(selinux/selinux.h,[ > + SELINUX_LIBS="-lselinux"],[ > + SELINUX_LIBS=""]) > +AC_SUBST(SELINUX_LIBS) > +]) > > --- testcases/kernel/fs/proc/Makefile.orig 2009-01-24 18:56:50.064650109 +0800 > +++ testcases/kernel/fs/proc/Makefile 2009-01-25 02:00:24.316649805 +0800 > @@ -16,12 +16,10 @@ > # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > # > > -########################################################################### > -# name of file : Makefile # > -# description : make(1) description file for the send(2) tests. # > -########################################################################### > -CFLAGS+= -I../../../../include > -LOADLIBES+= -L../../../../lib -lltp > +include ../../../../config.mk > + > +CFLAGS+= -I../../../../include -Wall > +LDLIBS+= -L../../../../lib -lltp $(SELINUX_LIBS) > > SRCS=$(wildcard *.c) > TARGETS=$(patsubst %.c,%,$(SRCS)) > @@ -33,5 +31,3 @@ > > clean: > rm -f $(TARGETS) > - > - > > --- configure.ac.orig 2009-01-24 16:41:35.894653037 +0800 > +++ configure.ac 2009-01-24 16:43:14.064654299 +0800 > @@ -18,5 +18,6 @@ > LTP_CHECK_SYSCALL_EVENTFD > LTP_CHECK_SYSCALL_MODIFY_LDT > LTP_CHECK_SYSCALL_SIGNALFD > +LTP_CHECK_SELINUX > > AC_OUTPUT > > --- config.mk.in.orig 2009-01-24 19:01:43.472650122 +0800 > +++ config.mk.in 2009-01-24 19:03:11.001651581 +0800 > @@ -7,3 +7,4 @@ > LDFLAGS = @LDFLAGS@ > > AIO_LIBS = @AIO_LIBS@ > +SELINUX_LIBS = @SELINUX_LIBS@ > Hi CAI Qian, Thanks for the patch to add the support for identifying and skipping the read errors when selinux enabled/disabled, but we could still get the EINVAL return value, while reading the read interface not supported by the current LSM, the user is using like AppArmor. To make it generic can we just skip reading the list of files, if they return EINVAL or else we have to support checking of different LSM's and add support for each of them individually. Agree that the coverage of the testcase is going to be reduced. It will be reduced more because the list which we are taking care is incomplete, we could need to add other files to the list like nfs to be skipped. Sending another patch which will ignore the file if it returns EINVAL or else throw warning. Please do not remove the cc list, while replying. --- testcases/kernel/fs/proc/proc01.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) Index: b/testcases/kernel/fs/proc/proc01.c =================================================================== --- a/testcases/kernel/fs/proc/proc01.c +++ b/testcases/kernel/fs/proc/proc01.c @@ -99,6 +99,16 @@ const char error_nonblock[][PATH_MAX] = "" }; +/* Files list to be skipped in the /proc file system, if the security + * modules does not support the /proc read interface. + * The list is not complete*/ +const char error_nolsminterface[][PATH_MAX] = +{ + "/proc/self/attr/*", + "/proc/self/task/[0-9]*/attr/*", + "", +}; + /* Verify expected failures, and then let the test to continue. */ int found_errno(const char *syscall, const char *obj, int tmperr) { @@ -289,8 +299,12 @@ int readproc(const char *obj) tmperr = errno; if (!found_errno("read", obj, tmperr)) { + if (errno == EINVAL) { + for (i = 0; error_nolsminterface[i][0] != '\0'; i++) + if (!fnmatch(error_nolsminterface [i], obj, FNM_PATHNAME)) + tst_resm(TINFO, "%s: LSM does not support read Interface", obj); /* ignore no perm (not root) and no process (terminated) errors */ - if (errno != EACCES && errno != ESRCH) { + } else if (errno != EACCES && errno != ESRCH) { tst_resm(TFAIL, "%s: read: %s", obj, strerror(errno)); close(fd); -- Thanks & Regards, Kamalesh Babulal, Linux Technology Center, IBM, ISTL. |
From: CAI Q. <ca...@cc...> - 2009-01-28 09:59:33
|
Hi, --- On Wed, 1/28/09, Kamalesh Babulal <kam...@li...> wrote: > From: Kamalesh Babulal <kam...@li...> > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > To: "CAI Qian" <ca...@cc...> > Cc: ltp...@li..., sd...@ty..., se...@us..., su...@li..., aa...@li... > Date: Wednesday, January 28, 2009, 2:31 PM > * CAI Qian <ca...@cc...> [2009-01-24 10:10:52]: > > > Hi, > > > > The following patch addes checking for SELinux. If it > is enabled, the > > following entries are expected to be read > successfully, > > > > /proc/self/attr/* > > /proc/self/task/[0-9]*/attr/* > > > > Otherwise, expecting read(2) return -1 with -EINVAL. > > > > It can be applied on the top of previously sent patch > with the title, > > > > [PATCH] proc01: /proc/ppc64/rtas/error_log: read: > Invalid argument > > > > Version 1 is broken. > > > > Signed-off-by: CAI Qian <ca...@cc...> > > > > --- testcases/kernel/fs/proc/proc01.c.p1 2009-01-24 > 19:08:51.843650731 +0800 > > +++ testcases/kernel/fs/proc/proc01.c 2009-01-25 > 02:06:00.001650743 +0800 > > @@ -25,6 +25,8 @@ > > * > > */ > > > > +#include "config.h" > > + > > #include <errno.h> /* for errno */ > > #include <stdio.h> /* for NULL */ > > #include <stdlib.h> /* for malloc() */ > > @@ -37,6 +39,10 @@ > > #include <fcntl.h> > > #include <fnmatch.h> > > > > +#ifdef HAVE_SELINUX_SELINUX_H > > +#include <selinux/selinux.h> > > +#endif > > + > > #include "test.h" > > #include "usctest.h" > > > > @@ -89,9 +95,23 @@ > > {"read", "/proc/self/mem", > EIO}, > > {"read", > "/proc/self/task/[0-9]*/mem", EIO}, > > {"read", > "/proc/ppc64/rtas/error_log", EINVAL}, > > + {"read", "/proc/self/attr/*", > EINVAL}, > > + {"read", > "/proc/self/task/[0-9]*/attr/*", EINVAL}, > > {"", "", 0} > > }; > > > > +#ifdef HAVE_SELINUX_SELINUX_H > > +/* If SELinux is enabled, the following entries > should be read > > + successfully. Note: SELinux libraries and headers > should be installed > > + for the test to read those files. Otherwise, they > will be skipped! */ > > +const char selinux_should_work[][PATH_MAX] = > > + { > > + "/proc/self/attr/*", > > + "/proc/self/task/[0-9]*/attr/*", > > + "" > > + }; > > +#endif > > + > > /* Known files that does not honor O_NONBLOCK, so > they will hang > > the test while being read.*/ > > const char error_nonblock[][PATH_MAX] = > > @@ -105,6 +125,19 @@ > > { > > int i; > > > > +/* Should not see any error for certain entries if > SELinux is enabled. */ > > +#ifdef HAVE_SELINUX_SELINUX_H > > + if (is_selinux_enabled()) > > + { > > + for (i = 0; selinux_should_work[i][0] != > '\0'; i++) > > + { > > + if (!strcmp(obj, selinux_should_work[i]) > > + || !fnmatch(selinux_should_work[i], > obj, FNM_PATHNAME)) > > + return 0; > > + } > > + } > > +#endif > > + > > for (i = 0; known_issues[i].err != 0; i++) > > if (tmperr == known_issues[i].err > > && (!strcmp(obj, > known_issues[i].file) > > @@ -143,6 +176,16 @@ > > TEST_PAUSE; > > > > tst_tmpdir(); > > + > > +#ifdef HAVE_SELINUX_SELINUX_H > > + if (is_selinux_enabled()) > > + tst_resm(TINFO, "SELinux is enabled."); > > + else > > + tst_resm(TINFO, "SELinux is disabled."); > > +#else > > + tst_resm(TINFO, > > + "unable to determine if SELinux is disabled or > not."); > > +#endif > > } > > > > void help() > > > > --- /dev/null 2009-01-24 15:26:18.326002642 +0800 > > +++ m4/ltp-selinux.m4 2009-01-24 19:56:54.660651164 > +0800 > > @@ -0,0 +1,29 @@ > > +dnl > > +dnl Copyright (c) Red Hat Inc., 2009 > > +dnl > > +dnl This program is free software; you can > redistribute it and/or > > +dnl modify it under the terms of the GNU General > Public License as > > +dnl published by the Free Software Foundation; either > version 2 of > > +dnl the License, or (at your option) any later > version. > > +dnl > > +dnl This program is distributed in the hope that it > will be useful, > > +dnl but WITHOUT ANY WARRANTY; without even the > implied warranty of > > +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR > PURPOSE. See > > +dnl the GNU General Public License for more details. > > +dnl > > +dnl You should have received a copy of the GNU > General Public License > > +dnl along with this program; if not, write to the > Free Software > > +dnl Foundation, Inc., 59 Temple Place, Suite 330, > Boston, MA 02111-1307 > > +dnl USA > > + > > +dnl > > +dnl LTP_CHECK_SELINUX > > +dnl ---------------------------- > > +dnl > > +AC_DEFUN([LTP_CHECK_SELINUX], > > +[dnl > > +AC_CHECK_HEADERS(selinux/selinux.h,[ > > + SELINUX_LIBS="-lselinux"],[ > > + SELINUX_LIBS=""]) > > +AC_SUBST(SELINUX_LIBS) > > +]) > > > > --- testcases/kernel/fs/proc/Makefile.orig 2009-01-24 > 18:56:50.064650109 +0800 > > +++ testcases/kernel/fs/proc/Makefile 2009-01-25 > 02:00:24.316649805 +0800 > > @@ -16,12 +16,10 @@ > > # Foundation, Inc., 59 Temple Place, Suite 330, > Boston, MA 02111-1307 USA > > # > > > > > -########################################################################### > > -# name of file : Makefile # > > -# description : make(1) description file for the > send(2) tests. # > > > -########################################################################### > > -CFLAGS+= -I../../../../include > > -LOADLIBES+= -L../../../../lib -lltp > > +include ../../../../config.mk > > + > > +CFLAGS+= -I../../../../include -Wall > > +LDLIBS+= -L../../../../lib -lltp $(SELINUX_LIBS) > > > > SRCS=$(wildcard *.c) > > TARGETS=$(patsubst %.c,%,$(SRCS)) > > @@ -33,5 +31,3 @@ > > > > clean: > > rm -f $(TARGETS) > > - > > - > > > > --- configure.ac.orig 2009-01-24 16:41:35.894653037 > +0800 > > +++ configure.ac 2009-01-24 16:43:14.064654299 +0800 > > @@ -18,5 +18,6 @@ > > LTP_CHECK_SYSCALL_EVENTFD > > LTP_CHECK_SYSCALL_MODIFY_LDT > > LTP_CHECK_SYSCALL_SIGNALFD > > +LTP_CHECK_SELINUX > > > > AC_OUTPUT > > > > --- config.mk.in.orig 2009-01-24 19:01:43.472650122 > +0800 > > +++ config.mk.in 2009-01-24 19:03:11.001651581 +0800 > > @@ -7,3 +7,4 @@ > > LDFLAGS = @LDFLAGS@ > > > > AIO_LIBS = @AIO_LIBS@ > > +SELINUX_LIBS = @SELINUX_LIBS@ > > > > Hi CAI Qian, > > Thanks for the patch to add the support for identifying > and skipping > the read errors when selinux enabled/disabled, but we could > still get the > EINVAL return value, while reading the read interface not > supported by the > current LSM, the user is using like AppArmor. Kamalesh Babulal, well, my approach is that anyone who cares about AppArmor can add a list of files should work to the code. it is fair that if different LSMs behave differently, we'll need different lists (selinux_should_work and apparmor_should_work) to deal with them. > To make it > generic can we > just skip reading the list of files, if they return EINVAL > or else we > have to support checking of different LSM's and add > support for each of > them individually. > Yes, but then you will still need to treat different LSMs differently. > Agree that the coverage of the testcase is going to be > reduced. It will be > reduced more because the list which we are taking care is > incomplete, Which ones are missing -- should return EINVAL with SELinux disabled? > we could need to add other files to the list like nfs to be > skipped. > Sending another patch which will ignore the file if it > returns EINVAL or else > throw warning. This patch won't able to catch attr/* entries return EINVAL while SELinux is enabled. It does not look like a good approach to me, because it is a test coverage regression. CAI Qian > > Please do not remove the cc list, while replying. > --- > testcases/kernel/fs/proc/proc01.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > Index: b/testcases/kernel/fs/proc/proc01.c > =================================================================== > --- a/testcases/kernel/fs/proc/proc01.c > +++ b/testcases/kernel/fs/proc/proc01.c > @@ -99,6 +99,16 @@ const char error_nonblock[][PATH_MAX] = > "" > }; > > +/* Files list to be skipped in the /proc file system, if > the security > + * modules does not support the /proc read interface. > + * The list is not complete*/ > +const char error_nolsminterface[][PATH_MAX] = > +{ > + "/proc/self/attr/*", > + "/proc/self/task/[0-9]*/attr/*", > + "", > +}; > + > /* Verify expected failures, and then let the test to > continue. */ > int found_errno(const char *syscall, const char *obj, int > tmperr) > { > @@ -289,8 +299,12 @@ int readproc(const char *obj) > tmperr = errno; > > if > (!found_errno("read", obj, tmperr)) { > + if (errno == EINVAL) { > + for (i = 0; error_nolsminterface[i][0] != > '\0'; i++) > + if (!fnmatch(error_nolsminterface [i], obj, > FNM_PATHNAME)) > + tst_resm(TINFO, "%s: LSM does not support > read Interface", obj); > /* ignore no perm (not root) and no process > (terminated) errors */ > - if (errno != EACCES && errno != ESRCH) { > + } else if (errno != EACCES && errno != ESRCH) > { > tst_resm(TFAIL, "%s: read: %s", obj, > strerror(errno)); > close(fd); > -- > Thanks & Regards, > Kamalesh Babulal, > Linux Technology Center, > IBM, ISTL. |
From: Serge E. H. <se...@us...> - 2009-01-28 14:57:33
|
Quoting CAI Qian (ca...@cc...): > Kamalesh Babulal, well, my approach is that anyone who cares about > AppArmor can add a list of files should work to the code. it is fair that if different LSMs behave differently, we'll need different lists > (selinux_should_work and apparmor_should_work) to deal with them. > > > To make it > > generic can we > > just skip reading the list of files, if they return EINVAL > > or else we > > have to support checking of different LSM's and add > > support for each of > > them individually. > > > > Yes, but then you will still need to treat different LSMs differently. > > > Agree that the coverage of the testcase is going to be > > reduced. It will be > > reduced more because the list which we are taking care is > > incomplete, > > Which ones are missing -- should return EINVAL with SELinux > disabled? > > > we could need to add other files to the list like nfs to be > > skipped. > > Sending another patch which will ignore the file if it > > returns EINVAL or else > > throw warning. > > This patch won't able to catch attr/* entries return > EINVAL while SELinux is enabled. It does not look like a good > approach to me, because it is a test coverage regression. > > CAI Qian So, just to try and think through this... If no LSM is enabled, the files should return -EINVAL. If they don't return -EINVAL, is that a situation we care about? What would it mean? If that is not a situation we care about, then we should simply ignore the files if selinux is disabled. If selinux is enabled, the user can run the selinux testsuite and it can test for proper return values. -serge |
From: CAI Q. <ca...@cc...> - 2009-01-28 15:25:47
|
Hi, --- On Wed, 1/28/09, Serge E. Hallyn <se...@us...> wrote: > From: Serge E. Hallyn <se...@us...> > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > To: "CAI Qian" <ca...@cc...> > Cc: "Kamalesh Babulal" <kam...@li...>, ltp...@li..., sd...@ty..., su...@li..., aa...@li... > Date: Wednesday, January 28, 2009, 10:57 PM > Quoting CAI Qian (ca...@cc...): > > Kamalesh Babulal, well, my approach is that anyone who > cares about > > AppArmor can add a list of files should work to the > code. it is fair that if different LSMs behave differently, > we'll need different lists > > (selinux_should_work and apparmor_should_work) to deal > with them. > > > > > To make it > > > generic can we > > > just skip reading the list of files, if they > return EINVAL > > > or else we > > > have to support checking of different LSM's > and add > > > support for each of > > > them individually. > > > > > > > Yes, but then you will still need to treat different > LSMs differently. > > > > > Agree that the coverage of the testcase is going > to be > > > reduced. It will be > > > reduced more because the list which we are taking > care is > > > incomplete, > > > > Which ones are missing -- should return EINVAL with > SELinux > > disabled? > > > > > we could need to add other files to the list like > nfs to be > > > skipped. > > > Sending another patch which will ignore the file > if it > > > returns EINVAL or else > > > throw warning. > > > > This patch won't able to catch attr/* entries > return > > EINVAL while SELinux is enabled. It does not look like > a good > > approach to me, because it is a test coverage > regression. > > > > CAI Qian > > So, just to try and think through this... If no LSM is > enabled, > the files should return -EINVAL. If they don't return > -EINVAL, > is that a situation we care about? What would it mean? > Yes, Stephen Smalley from National Security Agency of U.S. told it means "security modules (e.g. capability) don't support any of those interfaces", so if another errno is returned, it should be brought up to attention. CAI Qian > If that is not a situation we care about, then we should > simply > ignore the files if selinux is disabled. If selinux is > enabled, > the user can run the selinux testsuite and it can test for > proper > return values. > > -serge |
From: Serge E. H. <se...@us...> - 2009-01-28 15:57:21
|
Quoting CAI Qian (ca...@cc...): > Hi, > > > --- On Wed, 1/28/09, Serge E. Hallyn <se...@us...> wrote: > > > From: Serge E. Hallyn <se...@us...> > > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > To: "CAI Qian" <ca...@cc...> > > Cc: "Kamalesh Babulal" <kam...@li...>, ltp...@li..., sd...@ty..., su...@li..., aa...@li... > > Date: Wednesday, January 28, 2009, 10:57 PM > > Quoting CAI Qian (ca...@cc...): > > > Kamalesh Babulal, well, my approach is that anyone who > > cares about > > > AppArmor can add a list of files should work to the > > code. it is fair that if different LSMs behave differently, > > we'll need different lists > > > (selinux_should_work and apparmor_should_work) to deal > > with them. > > > > > > > To make it > > > > generic can we > > > > just skip reading the list of files, if they > > return EINVAL > > > > or else we > > > > have to support checking of different LSM's > > and add > > > > support for each of > > > > them individually. > > > > > > > > > > Yes, but then you will still need to treat different > > LSMs differently. > > > > > > > Agree that the coverage of the testcase is going > > to be > > > > reduced. It will be > > > > reduced more because the list which we are taking > > care is > > > > incomplete, > > > > > > Which ones are missing -- should return EINVAL with > > SELinux > > > disabled? > > > > > > > we could need to add other files to the list like > > nfs to be > > > > skipped. > > > > Sending another patch which will ignore the file > > if it > > > > returns EINVAL or else > > > > throw warning. > > > > > > This patch won't able to catch attr/* entries > > return > > > EINVAL while SELinux is enabled. It does not look like > > a good > > > approach to me, because it is a test coverage > > regression. > > > > > > CAI Qian > > > > So, just to try and think through this... If no LSM is > > enabled, > > the files should return -EINVAL. If they don't return > > -EINVAL, > > is that a situation we care about? What would it mean? > > > > Yes, Stephen Smalley from National Security Agency of U.S. told it > means "security modules (e.g. capability) don't support any of those > interfaces", so if another errno is returned, it should be brought up > to attention. Obviously the correct behavior depends upon the security subsystem, but you're finagling the checks into fs-specific checks. It seems to me that if you're going to check for correct return values from these functions, you should do so under testcases/kernel/security. -serge |
From: CAI Q. <ca...@cc...> - 2009-01-28 17:00:13
|
Hi, --- On Wed, 1/28/09, Serge E. Hallyn <se...@us...> wrote: > From: Serge E. Hallyn <se...@us...> > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > To: "CAI Qian" <ca...@cc...> > Cc: "Kamalesh Babulal" <kam...@li...>, ltp...@li..., sd...@ty..., su...@li..., aa...@li... > Date: Wednesday, January 28, 2009, 11:50 PM > Quoting CAI Qian (ca...@cc...): > > Hi, > > > > > > --- On Wed, 1/28/09, Serge E. Hallyn > <se...@us...> wrote: > > > > > From: Serge E. Hallyn <se...@us...> > > > Subject: Re: [LTP] [PATCH] proc01: SELinux with > attr/* Interface - version 2 > > > To: "CAI Qian" <ca...@cc...> > > > Cc: "Kamalesh Babulal" > <kam...@li...>, ltp...@li..., > sd...@ty..., su...@li..., > aa...@li... > > > Date: Wednesday, January 28, 2009, 10:57 PM > > > Quoting CAI Qian (ca...@cc...): > > > > Kamalesh Babulal, well, my approach is that > anyone who > > > cares about > > > > AppArmor can add a list of files should work > to the > > > code. it is fair that if different LSMs behave > differently, > > > we'll need different lists > > > > (selinux_should_work and > apparmor_should_work) to deal > > > with them. > > > > > > > > > To make it > > > > > generic can we > > > > > just skip reading the list of files, if > they > > > return EINVAL > > > > > or else we > > > > > have to support checking of different > LSM's > > > and add > > > > > support for each of > > > > > them individually. > > > > > > > > > > > > > Yes, but then you will still need to treat > different > > > LSMs differently. > > > > > > > > > Agree that the coverage of the > testcase is going > > > to be > > > > > reduced. It will be > > > > > reduced more because the list which we > are taking > > > care is > > > > > incomplete, > > > > > > > > Which ones are missing -- should return > EINVAL with > > > SELinux > > > > disabled? > > > > > > > > > we could need to add other files to the > list like > > > nfs to be > > > > > skipped. > > > > > Sending another patch which will ignore > the file > > > if it > > > > > returns EINVAL or else > > > > > throw warning. > > > > > > > > This patch won't able to catch attr/* > entries > > > return > > > > EINVAL while SELinux is enabled. It does not > look like > > > a good > > > > approach to me, because it is a test > coverage > > > regression. > > > > > > > > CAI Qian > > > > > > So, just to try and think through this... If no > LSM is > > > enabled, > > > the files should return -EINVAL. If they > don't return > > > -EINVAL, > > > is that a situation we care about? What would it > mean? > > > > > > > Yes, Stephen Smalley from National Security Agency of > U.S. told it > > means "security modules (e.g. capability) > don't support any of those > > interfaces", so if another errno is returned, it > should be brought up > > to attention. > > Obviously the correct behavior depends upon the security > subsystem, > but you're finagling the checks into fs-specific > checks. > > It seems to me that if you're going to check for > correct return > values from these functions, you should do so under > testcases/kernel/security. Well, it is fine to put some of those checks under another separate test, but it looks to me that it is probably easier and simpler here to consider those two tightly coupled component all together in this case. Because the test should not simple blindly skip those entries due to the reasons I have stated before, and people may run this test with SELinux on/off or AppArmor on/off (not consider other Linux security modules) as those are all valid test environment setup, we'll then have 3 different unique conditions: 1. SELinux on (attr/* read successfuly) 2. AppArmor on (???) 3. SELinux off and AppArmor off (attr/* read with -EINVAL) As the result, the above checking code will need to be present in both proc01 and a new test. CAI Qian > > -serge |
From: Stephen S. <sd...@ty...> - 2009-01-28 17:52:05
|
On Wed, 2009-01-28 at 09:00 -0800, CAI Qian wrote: > Hi, > > > --- On Wed, 1/28/09, Serge E. Hallyn <se...@us...> wrote: > > > From: Serge E. Hallyn <se...@us...> > > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > To: "CAI Qian" <ca...@cc...> > > Cc: "Kamalesh Babulal" <kam...@li...>, ltp...@li..., sd...@ty..., su...@li..., aa...@li... > > Date: Wednesday, January 28, 2009, 11:50 PM > > Quoting CAI Qian (ca...@cc...): > > > Hi, > > > > > > > > > --- On Wed, 1/28/09, Serge E. Hallyn > > <se...@us...> wrote: > > > > > > > From: Serge E. Hallyn <se...@us...> > > > > Subject: Re: [LTP] [PATCH] proc01: SELinux with > > attr/* Interface - version 2 > > > > To: "CAI Qian" <ca...@cc...> > > > > Cc: "Kamalesh Babulal" > > <kam...@li...>, ltp...@li..., > > sd...@ty..., su...@li..., > > aa...@li... > > > > Date: Wednesday, January 28, 2009, 10:57 PM > > > > Quoting CAI Qian (ca...@cc...): > > > > > Kamalesh Babulal, well, my approach is that > > anyone who > > > > cares about > > > > > AppArmor can add a list of files should work > > to the > > > > code. it is fair that if different LSMs behave > > differently, > > > > we'll need different lists > > > > > (selinux_should_work and > > apparmor_should_work) to deal > > > > with them. > > > > > > > > > > > To make it > > > > > > generic can we > > > > > > just skip reading the list of files, if > > they > > > > return EINVAL > > > > > > or else we > > > > > > have to support checking of different > > LSM's > > > > and add > > > > > > support for each of > > > > > > them individually. > > > > > > > > > > > > > > > > Yes, but then you will still need to treat > > different > > > > LSMs differently. > > > > > > > > > > > Agree that the coverage of the > > testcase is going > > > > to be > > > > > > reduced. It will be > > > > > > reduced more because the list which we > > are taking > > > > care is > > > > > > incomplete, > > > > > > > > > > Which ones are missing -- should return > > EINVAL with > > > > SELinux > > > > > disabled? > > > > > > > > > > > we could need to add other files to the > > list like > > > > nfs to be > > > > > > skipped. > > > > > > Sending another patch which will ignore > > the file > > > > if it > > > > > > returns EINVAL or else > > > > > > throw warning. > > > > > > > > > > This patch won't able to catch attr/* > > entries > > > > return > > > > > EINVAL while SELinux is enabled. It does not > > look like > > > > a good > > > > > approach to me, because it is a test > > coverage > > > > regression. > > > > > > > > > > CAI Qian > > > > > > > > So, just to try and think through this... If no > > LSM is > > > > enabled, > > > > the files should return -EINVAL. If they > > don't return > > > > -EINVAL, > > > > is that a situation we care about? What would it > > mean? > > > > > > > > > > Yes, Stephen Smalley from National Security Agency of > > U.S. told it > > > means "security modules (e.g. capability) > > don't support any of those > > > interfaces", so if another errno is returned, it > > should be brought up > > > to attention. > > > > Obviously the correct behavior depends upon the security > > subsystem, > > but you're finagling the checks into fs-specific > > checks. > > > > It seems to me that if you're going to check for > > correct return > > values from these functions, you should do so under > > testcases/kernel/security. > > Well, it is fine to put some of those checks under another separate > test, but it looks to me that it is probably easier and simpler here > to consider those two tightly coupled component all together in this > case. Because the test should not simple blindly skip those entries > due to the reasons I have stated before, and people may run this test > with SELinux on/off or AppArmor on/off (not consider other Linux > security modules) as those are all valid test environment setup, we'll > then have 3 different unique conditions: > > 1. SELinux on (attr/* read successfuly) > 2. AppArmor on (???) > 3. SELinux off and AppArmor off (attr/* read with -EINVAL) > > As the result, the above checking code will need to be present in both > proc01 and a new test. AppArmor (and Smack) support a subset of the attr/* files; in particular, they only support the attr/current file. The rest will return -1 with errno EINVAL upon read(2) calls. CONFIG_SECURITY=n completely omits the proc/self/attr subdirectory. For CONFIG_SECURITY=y, it might be nice if modules could selectively register support for a subset of the files and only have those files appear in the subdirectory, but that doesn't exist today. That would avoid proc01 even trying to call open() on the unsupported files. Of course, even if someone made that change today, you'd still want the ltp to work on existing kernels. BTW, the proc01 test is fairly limited in what it can truly test, given that it knows nothing about the semantics of the individual proc nodes and thus cannot assess whether the result is correct (it can tell whether read() succeeded or failed, but not whether the returned value is what one expects - that becomes specific to the particular proc node, and in the case of /proc/self/attr, it becomes specific not only to the particular security module but even to the particular policy in use). The selinux tests naturally make use of /proc/self/attr as part of exercising SELinux, both via calls to the libselinux interfaces and via use of the SELinux options to coreutils. Those tests can leverage their knowledge of SELinux and of the test policy to make more meaningful decisions about the results. -- Stephen Smalley National Security Agency |
From: Serge E. H. <se...@us...> - 2009-01-28 17:13:24
|
Quoting CAI Qian (ca...@cc...): > Hi, > > > --- On Wed, 1/28/09, Serge E. Hallyn <se...@us...> wrote: > > > From: Serge E. Hallyn <se...@us...> > > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > To: "CAI Qian" <ca...@cc...> > > Cc: "Kamalesh Babulal" <kam...@li...>, ltp...@li..., sd...@ty..., su...@li..., aa...@li... > > Date: Wednesday, January 28, 2009, 11:50 PM > > Quoting CAI Qian (ca...@cc...): > > > Hi, > > > > > > > > > --- On Wed, 1/28/09, Serge E. Hallyn > > <se...@us...> wrote: > > > > > > > From: Serge E. Hallyn <se...@us...> > > > > Subject: Re: [LTP] [PATCH] proc01: SELinux with > > attr/* Interface - version 2 > > > > To: "CAI Qian" <ca...@cc...> > > > > Cc: "Kamalesh Babulal" > > <kam...@li...>, ltp...@li..., > > sd...@ty..., su...@li..., > > aa...@li... > > > > Date: Wednesday, January 28, 2009, 10:57 PM > > > > Quoting CAI Qian (ca...@cc...): > > > > > Kamalesh Babulal, well, my approach is that > > anyone who > > > > cares about > > > > > AppArmor can add a list of files should work > > to the > > > > code. it is fair that if different LSMs behave > > differently, > > > > we'll need different lists > > > > > (selinux_should_work and > > apparmor_should_work) to deal > > > > with them. > > > > > > > > > > > To make it > > > > > > generic can we > > > > > > just skip reading the list of files, if > > they > > > > return EINVAL > > > > > > or else we > > > > > > have to support checking of different > > LSM's > > > > and add > > > > > > support for each of > > > > > > them individually. > > > > > > > > > > > > > > > > Yes, but then you will still need to treat > > different > > > > LSMs differently. > > > > > > > > > > > Agree that the coverage of the > > testcase is going > > > > to be > > > > > > reduced. It will be > > > > > > reduced more because the list which we > > are taking > > > > care is > > > > > > incomplete, > > > > > > > > > > Which ones are missing -- should return > > EINVAL with > > > > SELinux > > > > > disabled? > > > > > > > > > > > we could need to add other files to the > > list like > > > > nfs to be > > > > > > skipped. > > > > > > Sending another patch which will ignore > > the file > > > > if it > > > > > > returns EINVAL or else > > > > > > throw warning. > > > > > > > > > > This patch won't able to catch attr/* > > entries > > > > return > > > > > EINVAL while SELinux is enabled. It does not > > look like > > > > a good > > > > > approach to me, because it is a test > > coverage > > > > regression. > > > > > > > > > > CAI Qian > > > > > > > > So, just to try and think through this... If no > > LSM is > > > > enabled, > > > > the files should return -EINVAL. If they > > don't return > > > > -EINVAL, > > > > is that a situation we care about? What would it > > mean? > > > > > > > > > > Yes, Stephen Smalley from National Security Agency of > > U.S. told it > > > means "security modules (e.g. capability) > > don't support any of those > > > interfaces", so if another errno is returned, it > > should be brought up > > > to attention. > > > > Obviously the correct behavior depends upon the security > > subsystem, > > but you're finagling the checks into fs-specific > > checks. > > > > It seems to me that if you're going to check for > > correct return > > values from these functions, you should do so under > > testcases/kernel/security. > > Well, it is fine to put some of those checks under another separate > test, but it looks to me that it is probably easier and simpler here > to consider those two tightly coupled component all together in this > case. Because the test should not simple blindly skip those entries > due to the reasons I have stated before, and people may run this test > with SELinux on/off or AppArmor on/off (not consider other Linux > security modules) as those are all valid test environment setup, we'll > then have 3 different unique conditions: > > 1. SELinux on (attr/* read successfuly) > 2. AppArmor on (???) > 3. SELinux off and AppArmor off (attr/* read with -EINVAL) 4. TOMOYO on 5. Smack on ... > As the result, the above checking code will need to be present in both > proc01 and a new test. Then please stick to the simple suggestion from Stephen, keeping any selinux- (or any other lsm-)specific code out of proc01.c. Which may be what you're suggesting :) -serge |
From: Kamalesh B. <kam...@li...> - 2009-01-29 10:36:18
|
* Serge E. Hallyn <se...@us...> [2009-01-28 11:13:05]: > Quoting CAI Qian (ca...@cc...): > > Hi, > > > > > > --- On Wed, 1/28/09, Serge E. Hallyn <se...@us...> wrote: > > > > > From: Serge E. Hallyn <se...@us...> > > > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > > To: "CAI Qian" <ca...@cc...> > > > Cc: "Kamalesh Babulal" <kam...@li...>, ltp...@li..., sd...@ty..., su...@li..., aa...@li... > > > Date: Wednesday, January 28, 2009, 11:50 PM > > > Quoting CAI Qian (ca...@cc...): > > > > Hi, > > > > > > > > > > > > --- On Wed, 1/28/09, Serge E. Hallyn > > > <se...@us...> wrote: > > > > > > > > > From: Serge E. Hallyn <se...@us...> > > > > > Subject: Re: [LTP] [PATCH] proc01: SELinux with > > > attr/* Interface - version 2 > > > > > To: "CAI Qian" <ca...@cc...> > > > > > Cc: "Kamalesh Babulal" > > > <kam...@li...>, ltp...@li..., > > > sd...@ty..., su...@li..., > > > aa...@li... > > > > > Date: Wednesday, January 28, 2009, 10:57 PM > > > > > Quoting CAI Qian (ca...@cc...): > > > > > > Kamalesh Babulal, well, my approach is that > > > anyone who > > > > > cares about > > > > > > AppArmor can add a list of files should work > > > to the > > > > > code. it is fair that if different LSMs behave > > > differently, > > > > > we'll need different lists > > > > > > (selinux_should_work and > > > apparmor_should_work) to deal > > > > > with them. > > > > > > > > > > > > > To make it > > > > > > > generic can we > > > > > > > just skip reading the list of files, if > > > they > > > > > return EINVAL > > > > > > > or else we > > > > > > > have to support checking of different > > > LSM's > > > > > and add > > > > > > > support for each of > > > > > > > them individually. > > > > > > > > > > > > > > > > > > > Yes, but then you will still need to treat > > > different > > > > > LSMs differently. > > > > > > > > > > > > > Agree that the coverage of the > > > testcase is going > > > > > to be > > > > > > > reduced. It will be > > > > > > > reduced more because the list which we > > > are taking > > > > > care is > > > > > > > incomplete, > > > > > > > > > > > > Which ones are missing -- should return > > > EINVAL with > > > > > SELinux > > > > > > disabled? > > > > > > > > > > > > > we could need to add other files to the > > > list like > > > > > nfs to be > > > > > > > skipped. > > > > > > > Sending another patch which will ignore > > > the file > > > > > if it > > > > > > > returns EINVAL or else > > > > > > > throw warning. > > > > > > > > > > > > This patch won't able to catch attr/* Sorry send the wrong patch > > > entries > > > > > return > > > > > > EINVAL while SELinux is enabled. It does not > > > look like > > > > > a good > > > > > > approach to me, because it is a test > > > coverage > > > > > regression. > > > > > > > > > > > > CAI Qian > > > > > > > > > > So, just to try and think through this... If no > > > LSM is > > > > > enabled, > > > > > the files should return -EINVAL. If they > > > don't return > > > > > -EINVAL, > > > > > is that a situation we care about? What would it > > > mean? > > > > > > > > > > > > > Yes, Stephen Smalley from National Security Agency of > > > U.S. told it > > > > means "security modules (e.g. capability) > > > don't support any of those > > > > interfaces", so if another errno is returned, it > > > should be brought up > > > > to attention. > > > > > > Obviously the correct behavior depends upon the security > > > subsystem, > > > but you're finagling the checks into fs-specific > > > checks. > > > > > > It seems to me that if you're going to check for > > > correct return > > > values from these functions, you should do so under > > > testcases/kernel/security. > > > > Well, it is fine to put some of those checks under another separate > > test, but it looks to me that it is probably easier and simpler here > > to consider those two tightly coupled component all together in this > > case. Because the test should not simple blindly skip those entries > > due to the reasons I have stated before, and people may run this test > > with SELinux on/off or AppArmor on/off (not consider other Linux > > security modules) as those are all valid test environment setup, we'll > > then have 3 different unique conditions: > > > > 1. SELinux on (attr/* read successfuly) > > 2. AppArmor on (???) > > 3. SELinux off and AppArmor off (attr/* read with -EINVAL) > 4. TOMOYO on > 5. Smack on > ... > > > As the result, the above checking code will need to be present in both > > proc01 and a new test. > > Then please stick to the simple suggestion from Stephen, keeping > any selinux- (or any other lsm-)specific code out of proc01.c. > > Which may be what you're suggesting :) > > -serge We can just add the files related to LSM, to known failure list. We already check for their return value, if not EINVAL report test failure or else skip. Added the nfsd files to the list. --- testcases/kernel/fs/proc/proc01.c | 7 +++++++ 1 file changed, 7 insertions(+) Index: b/testcases/kernel/fs/proc/proc01.c =================================================================== --- a/testcases/kernel/fs/proc/proc01.c +++ b/testcases/kernel/fs/proc/proc01.c @@ -88,6 +88,13 @@ const Mapping known_issues[] = {"read", "/proc/xen/privcmd", EINVAL}, {"read", "/proc/self/mem", EIO}, {"read", "/proc/self/task/[0-9]*/mem", EIO}, + {"read", "/proc/self/attr/*", EINVAL}, + {"read", "/proc/self/task/[0-9]*/attr/*", EINVAL}, + {"read", "/proc/fs/nfsd/unlock_filesystem", EINVAL}, + {"read", "/proc/fs/nfsd/unlock_ip", EINVAL}, + {"read", "/proc/fs/nfsd/filehandle", EINVAL}, + {"read", "/proc/fs/nfsd/.getfs", EINVAL}, + {"read", "/proc/fs/nfsd/.getfd", EINVAL}, {"", "", 0} }; -- Thanks & Regards, Kamalesh Babulal, Linux Technology Center, IBM, ISTL. |
From: Serge E. H. <se...@us...> - 2009-01-29 16:28:08
|
Quoting Kamalesh Babulal (kam...@li...): > Sorry send the wrong patch Aaah. > > > > > > 1. SELinux on (attr/* read successfuly) > > > 2. AppArmor on (???) > > > 3. SELinux off and AppArmor off (attr/* read with -EINVAL) > > 4. TOMOYO on > > 5. Smack on > > ... > > > > > As the result, the above checking code will need to be present in both > > > proc01 and a new test. > > > > Then please stick to the simple suggestion from Stephen, keeping > > any selinux- (or any other lsm-)specific code out of proc01.c. > > > > Which may be what you're suggesting :) > > > > -serge > > We can just add the files related to LSM, to known failure list. We already check > for their return value, if not EINVAL report test failure or else skip. > Added the nfsd files to the list. > > --- > testcases/kernel/fs/proc/proc01.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > Index: b/testcases/kernel/fs/proc/proc01.c > =================================================================== > --- a/testcases/kernel/fs/proc/proc01.c > +++ b/testcases/kernel/fs/proc/proc01.c > @@ -88,6 +88,13 @@ const Mapping known_issues[] = > {"read", "/proc/xen/privcmd", EINVAL}, > {"read", "/proc/self/mem", EIO}, > {"read", "/proc/self/task/[0-9]*/mem", EIO}, > + {"read", "/proc/self/attr/*", EINVAL}, > + {"read", "/proc/self/task/[0-9]*/attr/*", EINVAL}, > + {"read", "/proc/fs/nfsd/unlock_filesystem", EINVAL}, > + {"read", "/proc/fs/nfsd/unlock_ip", EINVAL}, > + {"read", "/proc/fs/nfsd/filehandle", EINVAL}, > + {"read", "/proc/fs/nfsd/.getfs", EINVAL}, > + {"read", "/proc/fs/nfsd/.getfd", EINVAL}, Can't speak to the nfs parts, but putting the attr/* files there looks just right. thanks, -serge > {"", "", 0} > }; > > -- > Thanks & Regards, > Kamalesh Babulal, > Linux Technology Center, > IBM, ISTL. |
From: Subrata M. <su...@li...> - 2009-02-02 13:24:33
|
Thanks Serge, Cai, Kamalesh & Stephen. Regards-- Subrata On Thu, 2009-01-29 at 15:51 +0530, Kamalesh Babulal wrote: > * Serge E. Hallyn <se...@us...> [2009-01-28 11:13:05]: > > > Quoting CAI Qian (ca...@cc...): > > > Hi, > > > > > > > > > --- On Wed, 1/28/09, Serge E. Hallyn <se...@us...> wrote: > > > > > > > From: Serge E. Hallyn <se...@us...> > > > > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > > > To: "CAI Qian" <ca...@cc...> > > > > Cc: "Kamalesh Babulal" <kam...@li...>, ltp...@li..., sd...@ty..., su...@li..., aa...@li... > > > > Date: Wednesday, January 28, 2009, 11:50 PM > > > > Quoting CAI Qian (ca...@cc...): > > > > > Hi, > > > > > > > > > > > > > > > --- On Wed, 1/28/09, Serge E. Hallyn > > > > <se...@us...> wrote: > > > > > > > > > > > From: Serge E. Hallyn <se...@us...> > > > > > > Subject: Re: [LTP] [PATCH] proc01: SELinux with > > > > attr/* Interface - version 2 > > > > > > To: "CAI Qian" <ca...@cc...> > > > > > > Cc: "Kamalesh Babulal" > > > > <kam...@li...>, ltp...@li..., > > > > sd...@ty..., su...@li..., > > > > aa...@li... > > > > > > Date: Wednesday, January 28, 2009, 10:57 PM > > > > > > Quoting CAI Qian (ca...@cc...): > > > > > > > Kamalesh Babulal, well, my approach is that > > > > anyone who > > > > > > cares about > > > > > > > AppArmor can add a list of files should work > > > > to the > > > > > > code. it is fair that if different LSMs behave > > > > differently, > > > > > > we'll need different lists > > > > > > > (selinux_should_work and > > > > apparmor_should_work) to deal > > > > > > with them. > > > > > > > > > > > > > > > To make it > > > > > > > > generic can we > > > > > > > > just skip reading the list of files, if > > > > they > > > > > > return EINVAL > > > > > > > > or else we > > > > > > > > have to support checking of different > > > > LSM's > > > > > > and add > > > > > > > > support for each of > > > > > > > > them individually. > > > > > > > > > > > > > > > > > > > > > > Yes, but then you will still need to treat > > > > different > > > > > > LSMs differently. > > > > > > > > > > > > > > > Agree that the coverage of the > > > > testcase is going > > > > > > to be > > > > > > > > reduced. It will be > > > > > > > > reduced more because the list which we > > > > are taking > > > > > > care is > > > > > > > > incomplete, > > > > > > > > > > > > > > Which ones are missing -- should return > > > > EINVAL with > > > > > > SELinux > > > > > > > disabled? > > > > > > > > > > > > > > > we could need to add other files to the > > > > list like > > > > > > nfs to be > > > > > > > > skipped. > > > > > > > > Sending another patch which will ignore > > > > the file > > > > > > if it > > > > > > > > returns EINVAL or else > > > > > > > > throw warning. > > > > > > > > > > > > > > This patch won't able to catch attr/* > > Sorry send the wrong patch > > > > > entries > > > > > > return > > > > > > > EINVAL while SELinux is enabled. It does not > > > > look like > > > > > > a good > > > > > > > approach to me, because it is a test > > > > coverage > > > > > > regression. > > > > > > > > > > > > > > CAI Qian > > > > > > > > > > > > So, just to try and think through this... If no > > > > LSM is > > > > > > enabled, > > > > > > the files should return -EINVAL. If they > > > > don't return > > > > > > -EINVAL, > > > > > > is that a situation we care about? What would it > > > > mean? > > > > > > > > > > > > > > > > Yes, Stephen Smalley from National Security Agency of > > > > U.S. told it > > > > > means "security modules (e.g. capability) > > > > don't support any of those > > > > > interfaces", so if another errno is returned, it > > > > should be brought up > > > > > to attention. > > > > > > > > Obviously the correct behavior depends upon the security > > > > subsystem, > > > > but you're finagling the checks into fs-specific > > > > checks. > > > > > > > > It seems to me that if you're going to check for > > > > correct return > > > > values from these functions, you should do so under > > > > testcases/kernel/security. > > > > > > Well, it is fine to put some of those checks under another separate > > > test, but it looks to me that it is probably easier and simpler here > > > to consider those two tightly coupled component all together in this > > > case. Because the test should not simple blindly skip those entries > > > due to the reasons I have stated before, and people may run this test > > > with SELinux on/off or AppArmor on/off (not consider other Linux > > > security modules) as those are all valid test environment setup, we'll > > > then have 3 different unique conditions: > > > > > > 1. SELinux on (attr/* read successfuly) > > > 2. AppArmor on (???) > > > 3. SELinux off and AppArmor off (attr/* read with -EINVAL) > > 4. TOMOYO on > > 5. Smack on > > ... > > > > > As the result, the above checking code will need to be present in both > > > proc01 and a new test. > > > > Then please stick to the simple suggestion from Stephen, keeping > > any selinux- (or any other lsm-)specific code out of proc01.c. > > > > Which may be what you're suggesting :) > > > > -serge > > We can just add the files related to LSM, to known failure list. We already check > for their return value, if not EINVAL report test failure or else skip. > Added the nfsd files to the list. > > --- > testcases/kernel/fs/proc/proc01.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > Index: b/testcases/kernel/fs/proc/proc01.c > =================================================================== > --- a/testcases/kernel/fs/proc/proc01.c > +++ b/testcases/kernel/fs/proc/proc01.c > @@ -88,6 +88,13 @@ const Mapping known_issues[] = > {"read", "/proc/xen/privcmd", EINVAL}, > {"read", "/proc/self/mem", EIO}, > {"read", "/proc/self/task/[0-9]*/mem", EIO}, > + {"read", "/proc/self/attr/*", EINVAL}, > + {"read", "/proc/self/task/[0-9]*/attr/*", EINVAL}, > + {"read", "/proc/fs/nfsd/unlock_filesystem", EINVAL}, > + {"read", "/proc/fs/nfsd/unlock_ip", EINVAL}, > + {"read", "/proc/fs/nfsd/filehandle", EINVAL}, > + {"read", "/proc/fs/nfsd/.getfs", EINVAL}, > + {"read", "/proc/fs/nfsd/.getfd", EINVAL}, > {"", "", 0} > }; > |
From: CAI Q. <ca...@cc...> - 2009-02-03 03:23:12
|
Hi, --- On Mon, 2/2/09, Subrata Modak <su...@li...> wrote: > From: Subrata Modak <su...@li...> > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > To: "Kamalesh Babulal" <kam...@li...>, "Serge E. Hallyn" <se...@us...>, "CAI Qian" <ca...@cc...>, sd...@ty... > Cc: ltp...@li..., aa...@li... > Date: Monday, February 2, 2009, 9:24 PM > Thanks Serge, Cai, Kamalesh & Stephen. > The following patch should not be merged given that it did not address the problem completely that have been discussed in the previous mails. I'll send a new patch soon. CAI Qian > Regards-- > Subrata > > > > > We can just add the files related to LSM, to known failure list. We > > already check > > for their return value, if not EINVAL report test failure or else skip. > > Added the nfsd files to the list. > > > > --- > > testcases/kernel/fs/proc/proc01.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > Index: b/testcases/kernel/fs/proc/proc01.c > > > =================================================================== > > --- a/testcases/kernel/fs/proc/proc01.c > > +++ b/testcases/kernel/fs/proc/proc01.c > > @@ -88,6 +88,13 @@ const Mapping known_issues[] = > > {"read", "/proc/xen/privcmd", EINVAL}, > > {"read", "/proc/self/mem", EIO}, > > {"read", "/proc/self/task/[0-9]*/mem", EIO}, > > + {"read", "/proc/self/attr/*", EINVAL}, > > + {"read", "/proc/self/task/[0-9]*/attr/*", EINVAL}, > > + {"read", "/proc/fs/nfsd/unlock_filesystem", EINVAL}, > > + {"read", "/proc/fs/nfsd/unlock_ip", EINVAL}, > > + {"read", "/proc/fs/nfsd/filehandle", EINVAL}, > > + {"read", "/proc/fs/nfsd/.getfs", EINVAL}, > > + {"read", "/proc/fs/nfsd/.getfd", EINVAL}, > > {"", "", 0} > > }; > > |
From: CAI Q. <ca...@cc...> - 2009-01-29 16:33:15
|
Hi, ----- Original Message ---- > From: Kamalesh Babulal <kam...@li...> > To: Serge E. Hallyn <se...@us...> > Cc: CAI Qian <ca...@cc...>; ltp...@li...; sd...@ty...; su...@li...; aa...@li... > Sent: Thursday, January 29, 2009 6:21:49 PM > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > > We can just add the files related to LSM, to known failure list. We already > check for their return value, if not EINVAL report test failure or else skip. I am afraid this patch is incorrect. It is total valid that people may want to run this test in a SELinux enabled environment. For example, after applied the patch, we might see some day, INFO: /proc/self/attr/exec: EINVAL: known failure That is just incorrect, because this entry should return successfully in such a testing environment setup. It is also fairly normal that in order to support a different testing environment, we may add some conditional compilation code for then. In addition, for people like me have used the previous version of the test in a SELinux enabled environment, they will lost that kind of testing coverage for those entries (only return successfully), which is bad from the testing point of view. You can argue that this has already been testing in SELinux test suite, but it looks to me the test here has a different testing focus -- try to read every entry in /proc filesystem. Those entries belong to this filesystem, so we'll read them the same way. Who knows if those entries will not give us a hang or panic or behaving badly with certain read buffers? SELinux test suite may or may not catch those kind of bugs. CAI Qian > Added the nfsd files to the list. > > --- > testcases/kernel/fs/proc/proc01.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > Index: b/testcases/kernel/fs/proc/proc01.c > =================================================================== > --- a/testcases/kernel/fs/proc/proc01.c > +++ b/testcases/kernel/fs/proc/proc01.c > @@ -88,6 +88,13 @@ const Mapping known_issues[] = > {"read", "/proc/xen/privcmd", EINVAL}, > {"read", "/proc/self/mem", EIO}, > {"read", "/proc/self/task/[0-9]*/mem", EIO}, > + {"read", "/proc/self/attr/*", EINVAL}, > + {"read", "/proc/self/task/[0-9]*/attr/*", EINVAL}, > + {"read", "/proc/fs/nfsd/unlock_filesystem", EINVAL}, > + {"read", "/proc/fs/nfsd/unlock_ip", EINVAL}, > + {"read", "/proc/fs/nfsd/filehandle", EINVAL}, > + {"read", "/proc/fs/nfsd/.getfs", EINVAL}, > + {"read", "/proc/fs/nfsd/.getfd", EINVAL}, > {"", "", 0} > }; > > -- > Thanks & Regards, > Kamalesh Babulal, > Linux Technology Center, > IBM, ISTL. |
From: Serge E. H. <se...@us...> - 2009-01-29 16:52:59
|
Quoting CAI Qian (ca...@cc...): > Hi, > > > > ----- Original Message ---- > > From: Kamalesh Babulal <kam...@li...> > > To: Serge E. Hallyn <se...@us...> > > Cc: CAI Qian <ca...@cc...>; ltp...@li...; sd...@ty...; su...@li...; aa...@li... > > Sent: Thursday, January 29, 2009 6:21:49 PM > > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > > > > > We can just add the files related to LSM, to known failure list. We already > > check for their return value, if not EINVAL report test failure or else skip. > > I am afraid this patch is incorrect. It is total valid that people may want to run this > test in a SELinux enabled environment. For example, after applied the patch, we > might see some day, > > INFO: /proc/self/attr/exec: EINVAL: known failure > > That is just incorrect, because this entry should return successfully in such a testing > environment setup. > > It is also fairly normal that in order to support a different testing environment, we > may add some conditional compilation code for then. > > In addition, for people like me have used the previous version of the test in a > SELinux enabled environment, they will lost that kind of testing coverage for > those entries (only return successfully), which is bad from the testing point of > view. > > You can argue that this has already been testing in SELinux test suite, but it looks Yup. > to me the test here has a different testing focus -- try to read every entry in /proc > filesystem. Those entries belong to this filesystem, so we'll read them the same > way. Who knows if those entries will not give us a hang or panic or behaving > badly with certain read buffers? SELinux test suite may or may not catch those > kind of bugs. Then add tests in there... In fact, the logical course given your concern would be to write a kernel module defining an LSM allowing random long write to and reads from /proc/$$/attr/ so you can test the procfs bits of that API (if you could still write an LSM as a module :). It should be doable to push a 'debug' LSM into the upstream kernel which just serves to facilitate such testing. Anyway I've made my own position clear, and I think Kamalesh's patch implements precisely what Stephen suggested. thanks, -serge |
From: CAI Q. <ca...@cc...> - 2009-01-29 17:58:10
|
Hi, ----- Original Message ---- > From: Serge E. Hallyn <se...@us...> > To: CAI Qian <ca...@cc...> > Cc: Kamalesh Babulal <kam...@li...>; ltp...@li...; sd...@ty...; su...@li...; aa...@li... > Sent: Friday, January 30, 2009 12:52:52 AM > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > > You can argue that this has already been testing in SELinux test suite, but it > looks > > Yup. > > > to me the test here has a different testing focus -- try to read every entry in /proc > > filesystem. Those entries belong to this filesystem, so we'll read them the same > > way. Who knows if those entries will not give us a hang or panic or behaving > > badly with certain read buffers? SELinux test suite may or may not catch those > > kind of bugs. > > Then add tests in there... > No, they do not belong there, because we are focus on testing of proc filesystem here. The test case here should not care about the content it read, but rather what the kernel behaves when to read those files. > In fact, the logical course given your concern would be to write a > kernel module defining an LSM allowing random long write to and reads > from /proc/$$/attr/ so you can test the procfs bits of that API (if > you could still write an LSM as a module :). It should be doable to > push a 'debug' LSM into the upstream kernel which just serves to > facilitate such testing. > > Anyway I've made my own position clear, and I think Kamalesh's patch > implements precisely what Stephen suggested. > Stephen's suggestion is something we should take. I agree Kamalesh's patch has included his suggestion. Unfortunately, there are other issues with Kamalesh's patch that have been pointed out in the last email. CAI Qian > thanks, > -serge |
From: Serge E. H. <se...@us...> - 2009-01-29 18:24:08
|
Quoting CAI Qian (ca...@cc...): > Hi, > > > to me the test here has a different testing focus -- try to read every entry in /proc > > > filesystem. Those entries belong to this filesystem, so we'll read them the same > > > way. Who knows if those entries will not give us a hang or panic or behaving > > > badly with certain read buffers? SELinux test suite may or may not catch those > > > kind of bugs. > > > > Then add tests in there... > > > > No, they do not belong there, because we are focus on testing of proc > filesystem here. The test case here should not care about the content it > read, but rather what the kernel behaves when to read those files. Then the dummy lsm I suggested is the only way to really test what you want :) > > In fact, the logical course given your concern would be to write a > > kernel module defining an LSM allowing random long write to and reads > > from /proc/$$/attr/ so you can test the procfs bits of that API (if > > you could still write an LSM as a module :). It should be doable to > > push a 'debug' LSM into the upstream kernel which just serves to > > facilitate such testing. > > > > Anyway I've made my own position clear, and I think Kamalesh's patch > > implements precisely what Stephen suggested. > > > > Stephen's suggestion is something we should take. I agree Kamalesh's > patch has included his suggestion. Unfortunately, there are other issues > with Kamalesh's patch that have been pointed out in the last email. Sorry, I've looked back through the thread, but don't see the other issues you're talking about. -serge |
From: Stephen S. <sd...@ty...> - 2009-01-29 19:02:13
|
On Thu, 2009-01-29 at 12:23 -0600, Serge E. Hallyn wrote: > Quoting CAI Qian (ca...@cc...): > > Hi, > > > > to me the test here has a different testing focus -- try to read every entry in /proc > > > > filesystem. Those entries belong to this filesystem, so we'll read them the same > > > > way. Who knows if those entries will not give us a hang or panic or behaving > > > > badly with certain read buffers? SELinux test suite may or may not catch those > > > > kind of bugs. > > > > > > Then add tests in there... > > > > > > > No, they do not belong there, because we are focus on testing of proc > > filesystem here. The test case here should not care about the content it > > read, but rather what the kernel behaves when to read those files. > > Then the dummy lsm I suggested is the only way to really test what you > want :) That's not really a serious suggestion, right? If the proc01 test cannot be dependent on the presence/absence of any given security module (like SELinux), then it cannot be dependent on a fake test security module either as that would preclude running the test while SELinux was in fact enabled. > > > > In fact, the logical course given your concern would be to write a > > > kernel module defining an LSM allowing random long write to and reads > > > from /proc/$$/attr/ so you can test the procfs bits of that API (if > > > you could still write an LSM as a module :). It should be doable to > > > push a 'debug' LSM into the upstream kernel which just serves to > > > facilitate such testing. > > > > > > Anyway I've made my own position clear, and I think Kamalesh's patch > > > implements precisely what Stephen suggested. > > > > > > > Stephen's suggestion is something we should take. I agree Kamalesh's > > patch has included his suggestion. Unfortunately, there are other issues > > with Kamalesh's patch that have been pointed out in the last email. > > Sorry, I've looked back through the thread, but don't see the other > issues you're talking about. I think his concern is that it will obscure an actual bug in SELinux in the future. I don't care strongly about it either way - the selinux testsuite is what you should be using to exercise SELinux, and just booting and logging into a SELinux-enabled system will exercise many if not all of those interfaces. -- Stephen Smalley National Security Agency |
From: Serge E. H. <se...@us...> - 2009-01-29 19:47:26
|
Quoting Stephen Smalley (sd...@ty...): > On Thu, 2009-01-29 at 12:23 -0600, Serge E. Hallyn wrote: > > Quoting CAI Qian (ca...@cc...): > > > Hi, > > > > > to me the test here has a different testing focus -- try to read every entry in /proc > > > > > filesystem. Those entries belong to this filesystem, so we'll read them the same > > > > > way. Who knows if those entries will not give us a hang or panic or behaving > > > > > badly with certain read buffers? SELinux test suite may or may not catch those > > > > > kind of bugs. > > > > > > > > Then add tests in there... > > > > > > > > > > No, they do not belong there, because we are focus on testing of proc > > > filesystem here. The test case here should not care about the content it > > > read, but rather what the kernel behaves when to read those files. > > > > Then the dummy lsm I suggested is the only way to really test what you > > want :) > > That's not really a serious suggestion, right? If the proc01 test Well if he really wants to test the procfs part of it, then he should be testing reading and writing of various sizes... But no. > I think his concern is that it will obscure an actual bug in SELinux in > the future. I thought he was worried not about any SELinux bugs, but bugs in the way procfs passes data to/from selinux. > I don't care strongly about it either way - the selinux testsuite is > what you should be using to exercise SELinux, and just booting and > logging into a SELinux-enabled system will exercise many if not all of > those interfaces. Ok, I'm sorry, I'm dragging this out. I'll stop, I promise. -serge |
From: CAI Q. <ca...@cc...> - 2009-01-30 05:57:23
|
Hi, ----- Original Message ---- > From: Stephen Smalley <sd...@ty...> > To: Serge E. Hallyn <se...@us...> > Cc: CAI Qian <ca...@cc...>; ltp...@li...; aa...@li... > Sent: Friday, January 30, 2009 2:58:48 AM > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > > > > > > No, they do not belong there, because we are focus on testing of proc > > > filesystem here. The test case here should not care about the content it > > > read, but rather what the kernel behaves when to read those files. > > > > Then the dummy lsm I suggested is the only way to really test what you > > want :) > > That's not really a serious suggestion, right? If the proc01 test > cannot be dependent on the presence/absence of any given security module > (like SELinux), then it cannot be dependent on a fake test security > module either as that would preclude running the test while SELinux was > in fact enabled. > Exactly. The test should be able to run in different testing environment if possible, and expect correct results according to that particular environment if they differ. The test code just need a little bit careful to make it not overkill to maintain. > > > > > > In fact, the logical course given your concern would be to write a > > > > kernel module defining an LSM allowing random long write to and reads > > > > from /proc/$$/attr/ so you can test the procfs bits of that API (if > > > > you could still write an LSM as a module :). It should be doable to > > > > push a 'debug' LSM into the upstream kernel which just serves to > > > > facilitate such testing. > > > > > > > > Anyway I've made my own position clear, and I think Kamalesh's patch > > > > implements precisely what Stephen suggested. > > > > > > > > > > Stephen's suggestion is something we should take. I agree Kamalesh's > > > patch has included his suggestion. Unfortunately, there are other issues > > > with Kamalesh's patch that have been pointed out in the last email. > > > > Sorry, I've looked back through the thread, but don't see the other > > issues you're talking about. > > I think his concern is that it will obscure an actual bug in SELinux in > the future. > My main concerns of Kamalesh's patch are, * reduce testing coverage of the test with SELinux enabled testing environment, because all reads of attr/* files are not flagged EINVAL as errors any more. * false positive and misleading messages may happen on SELinux-enabled machine. If attr/* files returns EINVAL while SELinux is enabled happens in the future, it can be a bug in kernel or SELinux or both. > I don't care strongly about it either way - the selinux testsuite is > what you should be using to exercise SELinux, and just booting and > logging into a SELinux-enabled system will exercise many if not all of > those interfaces. > I agree. I suppose that proc01 is mainly to test how kernel behaves when to read those entries -- may use various length read buffers etc. Of course, people might use it differently. Someone may use it as a latency kernel for realtime kernel testing. It would be nice to keep that flexibiltiy other than saying that this test must be run with a particular kernel or LSM configuration. CAI Qian > -- > Stephen Smalley > National Security Agency |
From: CAI Q. <ca...@cc...> - 2009-01-30 06:25:33
|
Hi, ----- Original Message ---- > > > > > > > > No, they do not belong there, because we are focus on testing of proc > > > > filesystem here. The test case here should not care about the content it > > > > read, but rather what the kernel behaves when to read those files. > > > > > > Then the dummy lsm I suggested is the only way to really test what you > > > want :) > > > > That's not really a serious suggestion, right? If the proc01 test > > Well if he really wants to test the procfs part of it, then he should > be testing reading and writing of various sizes... But no. > There is a switch in proc01 test to pass a specified length of read buffer. Those entries can be tested the similar way by using that switch. > > I think his concern is that it will obscure an actual bug in SELinux in > > the future. > > I thought he was worried not about any SELinux bugs, but bugs in > the way procfs passes data to/from selinux. > Well, this test more looks like an end-user testing, so it should not concern too much about how internal working of those entries. From a end-user or test designer's expectation, those proc filesystems entries are present on the system, and have right permission to be able to read. If the system with SELinux is enabled, it expects the read(2) return 0 probably by using various length of read buffers. CAI Qian > > I don't care strongly about it either way - the selinux testsuite is > > what you should be using to exercise SELinux, and just booting and > > logging into a SELinux-enabled system will exercise many if not all of > > those interfaces. > > Ok, I'm sorry, I'm dragging this out. I'll stop, I promise. > > -serge |
From: Kamalesh B. <kam...@li...> - 2009-01-30 10:26:53
|
* Serge E. Hallyn <se...@us...> [2009-01-29 12:23:49]: > Quoting CAI Qian (ca...@cc...): > > Hi, > > > > to me the test here has a different testing focus -- try to read every entry in /proc > > > > filesystem. Those entries belong to this filesystem, so we'll read them the same > > > > way. Who knows if those entries will not give us a hang or panic or behaving > > > > badly with certain read buffers? SELinux test suite may or may not catch those > > > > kind of bugs. > > > > > > Then add tests in there... > > > > > > > No, they do not belong there, because we are focus on testing of proc > > filesystem here. The test case here should not care about the content it > > read, but rather what the kernel behaves when to read those files. > > Then the dummy lsm I suggested is the only way to really test what you > want :) > > > > In fact, the logical course given your concern would be to write a > > > kernel module defining an LSM allowing random long write to and reads > > > from /proc/$$/attr/ so you can test the procfs bits of that API (if > > > you could still write an LSM as a module :). It should be doable to > > > push a 'debug' LSM into the upstream kernel which just serves to > > > facilitate such testing. > > > > > > Anyway I've made my own position clear, and I think Kamalesh's patch > > > implements precisely what Stephen suggested. > > > > > > > Stephen's suggestion is something we should take. I agree Kamalesh's > > patch has included his suggestion. Unfortunately, there are other issues > > with Kamalesh's patch that have been pointed out in the last email. > > Sorry, I've looked back through the thread, but don't see the other > issues you're talking about. > > -serge Please find the results after applying the patch selinux disabled ----------------- proc01 0 INFO : /proc/acpi/event: open: known issue: Device or resource busy proc01 0 INFO : /proc/sys/net/ipv6/route/flush: is write-only. proc01 0 INFO : /proc/sys/net/ipv4/route/flush: is write-only. proc01 0 INFO : /proc/sys/fs/binfmt_misc/register: is write-only. proc01 0 INFO : /proc/sysrq-trigger: is write-only. proc01 0 INFO : /proc/kmsg: read: known issue: Resource temporarily unavailable proc01 0 INFO : /proc/self/task/2893/mem: read: known issue: Input/output error proc01 0 INFO : /proc/self/task/2893/attr/current: read: known issue: Invalid argument proc01 0 INFO : /proc/self/task/2893/attr/prev: read: known issue: Invalid argument proc01 0 INFO : /proc/self/task/2893/attr/exec: read: known issue: Invalid argument proc01 0 INFO : /proc/self/task/2893/attr/fscreate: read: known issue: Invalid argument proc01 0 INFO : /proc/self/task/2893/attr/keycreate: read: known issue: Invalid argument proc01 0 INFO : /proc/self/task/2893/attr/sockcreate: read: known issue: Invalid argument proc01 0 INFO : /proc/self/mem: read: known issue: Input/output error proc01 0 INFO : /proc/self/attr/current: read: known issue: Invalid argument proc01 0 INFO : /proc/self/attr/prev: read: known issue: Invalid argument proc01 0 INFO : /proc/self/attr/exec: read: known issue: Invalid argument proc01 0 INFO : /proc/self/attr/fscreate: read: known issue: Invalid argument proc01 0 INFO : /proc/self/attr/keycreate: read: known issue: Invalid argument proc01 0 INFO : /proc/self/attr/sockcreate: read: known issue: Invalid argument proc01 1 PASS : readproc() completed successfully, total read: 1095917 bytes, 885 objs selinux disabled (changing {"read", "/proc/self/task/[0-9]*/attr/*", EIO and {"read", "/proc/self/task/[0-9]*/attr/*", EIO}) ---------------- proc01 0 INFO : /proc/acpi/event: open: known issue: Device or resource busy proc01 0 INFO : /proc/sys/net/ipv6/route/flush: is write-only. proc01 0 INFO : /proc/sys/net/ipv4/route/flush: is write-only. proc01 0 INFO : /proc/sys/fs/binfmt_misc/register: is write-only. proc01 0 INFO : /proc/sysrq-trigger: is write-only. proc01 0 INFO : /proc/kmsg: read: known issue: Resource temporarily unavailable proc01 0 INFO : /proc/self/task/2991/mem: read: known issue: Input/output error proc01 1 FAIL : /proc/self/task/2991/attr/current: read: Invalid argument proc01 2 FAIL : /proc/self/task/2991/attr/prev: read: Invalid argument proc01 3 FAIL : /proc/self/task/2991/attr/exec: read: Invalid argument proc01 4 FAIL : /proc/self/task/2991/attr/fscreate: read: Invalid argument proc01 5 FAIL : /proc/self/task/2991/attr/keycreate: read: Invalid argument proc01 6 FAIL : /proc/self/task/2991/attr/sockcreate: read: Invalid argument proc01 0 INFO : /proc/self/mem: read: known issue: Input/output error proc01 7 FAIL : /proc/self/attr/current: read: Invalid argument proc01 8 FAIL : /proc/self/attr/prev: read: Invalid argument proc01 9 FAIL : /proc/self/attr/exec: read: Invalid argument proc01 10 FAIL : /proc/self/attr/fscreate: read: Invalid argument proc01 11 FAIL : /proc/self/attr/keycreate: read: Invalid argument proc01 12 FAIL : /proc/self/attr/sockcreate: read: Invalid argument proc01 13 FAIL : readproc() failed with 12 errors. selinux enabled ---------------- proc01 0 INFO : /proc/acpi/event: open: known issue: Device or resource busy proc01 0 INFO : /proc/sys/net/ipv6/route/flush: is write-only. proc01 0 INFO : /proc/sys/net/ipv4/route/flush: is write-only. proc01 0 INFO : /proc/sys/fs/binfmt_misc/register: is write-only. proc01 0 INFO : /proc/sysrq-trigger: is write-only. proc01 0 INFO : /proc/kmsg: read: known issue: Resource temporarily unavailable proc01 0 INFO : /proc/self/task/2875/mem: read: known issue: Input/output error proc01 0 INFO : /proc/self/mem: read: known issue: Input/output error proc01 1 PASS : readproc() completed successfully, total read: 1096865 bytes, 885 objs the EINVAL is returned only when the LSM is does not support the interface, and found_errno() checks for the know return value or else it handled the way the unknow error is hanlded. -- Thanks & Regards, Kamalesh Babulal, Linux Technology Center, IBM, ISTL. |
From: Serge E. H. <se...@us...> - 2009-01-30 16:54:50
|
Quoting Kamalesh Babulal (kam...@li...): > selinux enabled > ---------------- > proc01 0 INFO : /proc/acpi/event: open: known issue: Device or resource busy > proc01 0 INFO : /proc/sys/net/ipv6/route/flush: is write-only. > proc01 0 INFO : /proc/sys/net/ipv4/route/flush: is write-only. > proc01 0 INFO : /proc/sys/fs/binfmt_misc/register: is write-only. > proc01 0 INFO : /proc/sysrq-trigger: is write-only. > proc01 0 INFO : /proc/kmsg: read: known issue: Resource temporarily unavailable > proc01 0 INFO : /proc/self/task/2875/mem: read: known issue: Input/output error > proc01 0 INFO : /proc/self/mem: read: known issue: Input/output error > proc01 1 PASS : readproc() completed successfully, total read: 1096865 bytes, 885 objs > > the EINVAL is returned only when the LSM is does not support the > interface, and found_errno() checks for the know return value or else > it handled the way the unknow error is hanlded. Right, but I think CAI is concerned that if there is a regression with selinux enabled and it mistakenly returns -EINVAL this won't catch it. As Stephen pointed out, if that happens then you likely won't get a successful boot to begin with... thanks, -serge |
From: CAI Q. <ca...@cc...> - 2009-02-01 05:32:14
|
Hi, ----- Original Message ---- > From: Serge E. Hallyn <se...@us...> > To: Kamalesh Babulal <kam...@li...> > Cc: CAI Qian <ca...@cc...>; ltp...@li...; sd...@ty...; su...@li...; aa...@li... > Sent: Saturday, January 31, 2009 12:54:42 AM > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > > Quoting Kamalesh Babulal (kam...@li...): > > selinux enabled > > ---------------- > > proc01 0 INFO : /proc/acpi/event: open: known issue: Device or > resource busy > > proc01 0 INFO : /proc/sys/net/ipv6/route/flush: is write-only. > > proc01 0 INFO : /proc/sys/net/ipv4/route/flush: is write-only. > > proc01 0 INFO : /proc/sys/fs/binfmt_misc/register: is write-only. > > proc01 0 INFO : /proc/sysrq-trigger: is write-only. > > proc01 0 INFO : /proc/kmsg: read: known issue: Resource temporarily > unavailable > > proc01 0 INFO : /proc/self/task/2875/mem: read: known issue: > Input/output error > > proc01 0 INFO : /proc/self/mem: read: known issue: Input/output error > > proc01 1 PASS : readproc() completed successfully, total read: 1096865 > bytes, 885 objs > > > > the EINVAL is returned only when the LSM is does not support the > > interface, and found_errno() checks for the know return value or else > > it handled the way the unknow error is hanlded. > > Right, but I think CAI is concerned that if there is a regression with > selinux enabled and it mistakenly returns -EINVAL this won't catch it. > That is correct. In addition, if it happens, there will be some false positive and bad messages like, proc01 0 INFO: /proc/self/attr/exec: read: known issue: Invalid argument > As Stephen pointed out, if that happens then you likely won't get a > successful boot to begin with... > Perhaps, but by testing those entries the same way as other procfs entries here, we can catch EINVAL in same scenarios and facilities the test provided. For example, using different sizes of read buffers . CAI Qian > thanks, > -serge |
From: CAI Q. <ca...@cc...> - 2009-01-28 16:01:42
|
--- On Wed, 1/28/09, CAI Qian <ca...@cc...> wrote: > From: CAI Qian <ca...@cc...> > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* Interface - version 2 > To: "Serge E. Hallyn" <se...@us...> > Cc: ltp...@li..., aa...@li..., sd...@ty... > Date: Wednesday, January 28, 2009, 11:25 PM > Hi, > > > --- On Wed, 1/28/09, Serge E. Hallyn > <se...@us...> wrote: > > > From: Serge E. Hallyn <se...@us...> > > Subject: Re: [LTP] [PATCH] proc01: SELinux with attr/* > Interface - version 2 > > To: "CAI Qian" <ca...@cc...> > > Cc: "Kamalesh Babulal" > <kam...@li...>, ltp...@li..., > sd...@ty..., su...@li..., > aa...@li... > > Date: Wednesday, January 28, 2009, 10:57 PM > > Quoting CAI Qian (ca...@cc...): > > > Kamalesh Babulal, well, my approach is that > anyone who > > cares about > > > AppArmor can add a list of files should work to > the > > code. it is fair that if different LSMs behave > differently, > > we'll need different lists > > > (selinux_should_work and apparmor_should_work) to > deal > > with them. > > > > > > > To make it > > > > generic can we > > > > just skip reading the list of files, if they > > return EINVAL > > > > or else we > > > > have to support checking of different > LSM's > > and add > > > > support for each of > > > > them individually. > > > > > > > > > > Yes, but then you will still need to treat > different > > LSMs differently. > > > > > > > Agree that the coverage of the testcase is > going > > to be > > > > reduced. It will be > > > > reduced more because the list which we are > taking > > care is > > > > incomplete, > > > > > > Which ones are missing -- should return EINVAL > with > > SELinux > > > disabled? > > > > > > > we could need to add other files to the list > like > > nfs to be > > > > skipped. > > > > Sending another patch which will ignore the > file > > if it > > > > returns EINVAL or else > > > > throw warning. > > > > > > This patch won't able to catch attr/* entries > > return > > > EINVAL while SELinux is enabled. It does not look > like > > a good > > > approach to me, because it is a test coverage > > regression. > > > > > > CAI Qian > > > > So, just to try and think through this... If no LSM > is > > enabled, > > the files should return -EINVAL. If they don't > return > > -EINVAL, > > is that a situation we care about? What would it > mean? > > > > Yes, Stephen Smalley from National Security Agency of U.S. > told it > means "security modules (e.g. capability) don't > support any of those > interfaces", so if another errno is returned, it > should be brought up > to attention. > Here is the link for the email from Stephen Smalley that I was refer to, http://article.gmane.org/gmane.linux.ltp/7324 > CAI Qian > > > If that is not a situation we care about, then we > should > > simply > > ignore the files if selinux is disabled. If selinux > is > > enabled, > > the user can run the selinux testsuite and it can test > for > > proper > > return values. > > > > -serge > > ------------------------------------------------------------------------------ > This SF.net email is sponsored by: > SourcForge Community > SourceForge wants to tell your story. > http://p.sf.net/sfu/sf-spreadtheword > _______________________________________________ > Ltp-list mailing list > Ltp...@li... > https://lists.sourceforge.net/lists/listinfo/ltp-list |