| Message ID | 20200720194920.22784-1-ernunes@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/3] lib: add function to check for kernel lockdown | expand |
Hi Erico, Thanks for working on this fix. Comments as below: On Tue, Jul 21, 2020 at 3:50 AM Erico Nunes <ernunes@redhat.com> wrote: > Some syscalls are not available if the kernel is booted using the > 'lockdown' feature. That can cause some tests to report fail, showing > a message like: > > Lockdown: iopl01: iopl is restricted; see man kernel_lockdown.7 > > This patch adds a function that can be used by tests to check for this > case, so tests can be skipped rather than reporting a test failure. > > Signed-off-by: Erico Nunes <ernunes@redhat.com> > --- > include/tst_lockdown.h | 8 ++++++++ > include/tst_test.h | 1 + > lib/tst_lockdown.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+) > create mode 100644 include/tst_lockdown.h > create mode 100644 lib/tst_lockdown.c > > diff --git a/include/tst_lockdown.h b/include/tst_lockdown.h > new file mode 100644 > index 000000000..8db26d943 > --- /dev/null > +++ b/include/tst_lockdown.h > @@ -0,0 +1,8 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#ifndef TST_LOCKDOWN_H > +#define TST_LOCKDOWN_H > + > +void tst_lockdown_skip(void); > + > +#endif /* TST_LOCKDOWN_H */ > diff --git a/include/tst_test.h b/include/tst_test.h > index b84f7b9dd..b02de4597 100644 > --- a/include/tst_test.h > +++ b/include/tst_test.h > @@ -40,6 +40,7 @@ > #include "tst_hugepage.h" > #include "tst_assert.h" > #include "tst_cgroup.h" > +#include "tst_lockdown.h" > > /* > * Reports testcase result. > diff --git a/lib/tst_lockdown.c b/lib/tst_lockdown.c > new file mode 100644 > index 000000000..d57a6bdf3 > --- /dev/null > +++ b/lib/tst_lockdown.c > @@ -0,0 +1,28 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#define TST_NO_DEFAULT_MAIN > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <sys/mount.h> > + > +#include "tst_test.h" > +#include "tst_safe_macros.h" > +#include "tst_safe_stdio.h" > +#include "tst_lockdown.h" > + > +void tst_lockdown_skip(void) > Maybe renaming the function to tst_lockdown_enabled() is better? Then we can return 1 if confirm kernel under lockdown mode otherwise 0. +{ > + char line[BUFSIZ]; > + FILE *file; > + > + if (access("/sys/kernel/security/lockdown", F_OK) != 0) > After thinking over, I guess it's not enough to only check /sys/../lockdown file. Seems we need to consider the situation that system without supporting this file? i.e. Test on RHEL8 (no /sys/../lockdown file) with kernel parameter "lockdown" and got the restriction error too. # cat /proc/cmdline BOOT_IMAGE=(hd0,msdos1)/vmlinuz-4.18.0-226.el8.x86_64 root=/dev/mapper/rhel_bootp--73--3--209-root ro console=ttyS0,115200 ... lockdown # ll /sys/kernel/security/lockdown ls: cannot access '/sys/kernel/security/lockdown': No such file or directory # ./iopl01 ... iopl01.c:37: FAIL: iopl() failed for level 1, errno=1 : EPERM: EPERM (1) iopl01.c:37: FAIL: iopl() failed for level 2, errno=1 : EPERM: EPERM (1) > + return; > + > + file = SAFE_FOPEN("/sys/kernel/security/lockdown", "r"); > + fgets(line, sizeof(line), file); > + SAFE_FCLOSE(file); > + > + if (strstr(line, "[none]") == NULL) > + tst_brk(TCONF, "Kernel is locked down, skip this test."); > +} > -- > 2.26.2 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp > >
Thanks for the review. I'll address other comments soon, just an initial note below: On 7/21/20 9:46 AM, Li Wang wrote: > Maybe renaming the function to tst_lockdown_enabled() is better? Then we > can return 1 if confirm kernel under lockdown mode otherwise 0. > > +{ > + char line[BUFSIZ]; > + FILE *file; > + > + if (access("/sys/kernel/security/lockdown", F_OK) != 0) > > > After thinking over, I guess it's not enough to only check > /sys/../lockdown file. Seems we need to consider the situation that > system without supporting this file? > > i.e. > Test on RHEL8 (no /sys/../lockdown file) with kernel parameter > "lockdown" and got the restriction error too. > > # cat /proc/cmdline > BOOT_IMAGE=(hd0,msdos1)/vmlinuz-4.18.0-226.el8.x86_64 > root=/dev/mapper/rhel_bootp--73--3--209-root ro console=ttyS0,115200 > ... lockdown > > # ll /sys/kernel/security/lockdown > ls: cannot access '/sys/kernel/security/lockdown': No such file or directory To my understanding, the parameter to enable lockdown through kenrel parameters is "lockdown={integrity|confidentiality}", not just "lockdown", at least on upstream kernels: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aefcf2f4b58155d27340ba5f9ddbe9513da8286d If /sys/kernel/security/lockdown doesn't exist, I'm not sure there is much we can do easily, or that is worth doing now. I think it is ok to fall back and fail like it has been happening since the feature was merged upstream. I can't see a tweak that would enable the feature but not the sysfs file in the kernel source. Maybe that kernel only had partial support? Erico
Erico, On Tue, Jul 21, 2020 at 4:57 PM Erico Nunes <ernunes@redhat.com> wrote: > ... > > > Maybe renaming the function to tst_lockdown_enabled() is better? Then we > > can return 1 if confirm kernel under lockdown mode otherwise 0. > How do you think about this suggestion? ^^ Another reason to name it as tst_lockdown_enabled() is, we can give more flexible to test case, because not all tests need a simple skip in lockdown mode(in future). i.e. if (tst_lockdown_enabled()) { // skip or not, // do what they wanted in this mode } > After thinking over, I guess it's not enough to only check > > /sys/../lockdown file. Seems we need to consider the situation that > > system without supporting this file? > > > > i.e. > > Test on RHEL8 (no /sys/../lockdown file) with kernel parameter > > "lockdown" and got the restriction error too. > > > > # cat /proc/cmdline > > BOOT_IMAGE=(hd0,msdos1)/vmlinuz-4.18.0-226.el8.x86_64 > > root=/dev/mapper/rhel_bootp--73--3--209-root ro console=ttyS0,115200 > > ... lockdown > > > > # ll /sys/kernel/security/lockdown > > ls: cannot access '/sys/kernel/security/lockdown': No such file or > directory > > To my understanding, the parameter to enable lockdown through kenrel > parameters is "lockdown={integrity|confidentiality}", not just > "lockdown", at least on upstream kernels: > Good to know this. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=aefcf2f4b58155d27340ba5f9ddbe9513da8286d > > > If /sys/kernel/security/lockdown doesn't exist, I'm not sure there is > much we can do easily, or that is worth doing now. I think it is ok to > fall back and fail like it has been happening since the feature was > merged upstream. > Yes, it looks a bit tricky. > I can't see a tweak that would enable the feature but not the sysfs file > in the kernel source. Maybe that kernel only had partial support? > Seems you're right, there are many differences between mainline-kernel and some distros in lockdown code. The reason that some distribution (i.e RHEL, Ubuntu) partly customizes the LSM feature, it does not support lockdown features completely so far. But one point we're sure is that the /sys/kernel/../lockdown file was introduced from kernel-v5.4. So maybe we could simply do detect the /sys/kernel/../loackdown file as your patch, but adding an extra warning print when test failed on older than kernel-v5.4. Or, if others can provide a better way I'd happy to hear.
Hi! > Some syscalls are not available if the kernel is booted using the > 'lockdown' feature. That can cause some tests to report fail, showing > a message like: > > Lockdown: iopl01: iopl is restricted; see man kernel_lockdown.7 > > This patch adds a function that can be used by tests to check for this > case, so tests can be skipped rather than reporting a test failure. > > Signed-off-by: Erico Nunes <ernunes@redhat.com> > --- > include/tst_lockdown.h | 8 ++++++++ > include/tst_test.h | 1 + > lib/tst_lockdown.c | 28 ++++++++++++++++++++++++++++ > 3 files changed, 37 insertions(+) > create mode 100644 include/tst_lockdown.h > create mode 100644 lib/tst_lockdown.c > > diff --git a/include/tst_lockdown.h b/include/tst_lockdown.h > new file mode 100644 > index 000000000..8db26d943 > --- /dev/null > +++ b/include/tst_lockdown.h > @@ -0,0 +1,8 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#ifndef TST_LOCKDOWN_H > +#define TST_LOCKDOWN_H > + > +void tst_lockdown_skip(void); > + > +#endif /* TST_LOCKDOWN_H */ > diff --git a/include/tst_test.h b/include/tst_test.h > index b84f7b9dd..b02de4597 100644 > --- a/include/tst_test.h > +++ b/include/tst_test.h > @@ -40,6 +40,7 @@ > #include "tst_hugepage.h" > #include "tst_assert.h" > #include "tst_cgroup.h" > +#include "tst_lockdown.h" > > /* > * Reports testcase result. > diff --git a/lib/tst_lockdown.c b/lib/tst_lockdown.c > new file mode 100644 > index 000000000..d57a6bdf3 > --- /dev/null > +++ b/lib/tst_lockdown.c > @@ -0,0 +1,28 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#define TST_NO_DEFAULT_MAIN > + > +#include <stdio.h> > +#include <stdlib.h> > +#include <sys/mount.h> > + > +#include "tst_test.h" > +#include "tst_safe_macros.h" > +#include "tst_safe_stdio.h" > +#include "tst_lockdown.h" > + > +void tst_lockdown_skip(void) > +{ > + char line[BUFSIZ]; > + FILE *file; > + > + if (access("/sys/kernel/security/lockdown", F_OK) != 0) > + return; > + > + file = SAFE_FOPEN("/sys/kernel/security/lockdown", "r"); > + fgets(line, sizeof(line), file); The compiler complains that we haven't checked the return value here I guess that we can silence it with: if (!fgets(line, sizeof(line), file) return; > + SAFE_FCLOSE(file); > + > + if (strstr(line, "[none]") == NULL) > + tst_brk(TCONF, "Kernel is locked down, skip this test."); > +} > -- > 2.26.2 > > > -- > Mailing list info: https://lists.linux.it/listinfo/ltp
On 7/21/20 5:26 PM, Cyril Hrubis wrote: >> +void tst_lockdown_skip(void) >> +{ >> + char line[BUFSIZ]; >> + FILE *file; >> + >> + if (access("/sys/kernel/security/lockdown", F_OK) != 0) >> + return; >> + >> + file = SAFE_FOPEN("/sys/kernel/security/lockdown", "r"); >> + fgets(line, sizeof(line), file); > > The compiler complains that we haven't checked the return value here I > guess that we can silence it with: > > if (!fgets(line, sizeof(line), file) > return; > Thanks, will fix in v2.
On 7/21/20 3:19 PM, Li Wang wrote: > On Tue, Jul 21, 2020 at 4:57 PM Erico Nunes <ernunes@redhat.com > <mailto:ernunes@redhat.com>> wrote: > > ... > > > Maybe renaming the function to tst_lockdown_enabled() is better? > Then we > > can return 1 if confirm kernel under lockdown mode otherwise 0. > > > How do you think about this suggestion? ^^ > > Another reason to name it as tst_lockdown_enabled() is, we can give more > flexible > to test case, because not all tests need a simple skip in lockdown > mode(in future). > > i.e. > if (tst_lockdown_enabled()) { > // skip or not, > // do what they wanted in this mode > } I like this suggestion, I'll send v2 with this. > If /sys/kernel/security/lockdown doesn't exist, I'm not sure there is > much we can do easily, or that is worth doing now. I think it is ok to > fall back and fail like it has been happening since the feature was > merged upstream. > > > Yes, it looks a bit tricky. > > > I can't see a tweak that would enable the feature but not the sysfs file > in the kernel source. Maybe that kernel only had partial support? > > > Seems you're right, there are many differences between mainline-kernel > and some distros in lockdown code. The reason that some distribution > (i.e RHEL, Ubuntu) partly customizes the LSM feature, it does not support > lockdown features completely so far. > > But one point we're sure is that the /sys/kernel/../lockdown file was > introduced from kernel-v5.4. > > So maybe we could simply do detect the /sys/kernel/../loackdown file as > your patch, > but adding an extra warning print when test failed on older than > kernel-v5.4. I like the idea of the warning. The only thing to consider is that the warning would also show up on all old kernels that don't even support lockdown and then don't have the file. So would you suggest this message to be something like a tst_res(TWARN, ...) or TINFO or some other less noisy way? I also thought about limiting to some kernel version but that wouldn't work with distribution kernels like RHEL which have an earlier version number but also have the feature... Erico
Hi! > > So maybe we could simply do detect the /sys/kernel/../loackdown file as > > your patch, > > but adding an extra warning print when test failed on older than > > kernel-v5.4. > > I like the idea of the warning. The only thing to consider is that the > warning would also show up on all old kernels that don't even support > lockdown and then don't have the file. So would you suggest this message > to be something like a tst_res(TWARN, ...) or TINFO or some other less > noisy way? TWARN will cause the test to exit with non-zero status, which will probably show up as a failure in some environments, so I would go for TINFO. > I also thought about limiting to some kernel version but that wouldn't > work with distribution kernels like RHEL which have an earlier version > number but also have the feature... We also have an interface to match different kernel versions per distribution, have a look at tst_kern_exv structure in inotify04.c testcase.
On Wed, Jul 22, 2020 at 11:52 PM Erico Nunes <ernunes@redhat.com> wrote: > ... > > So maybe we could simply do detect the /sys/kernel/../loackdown file as > > your patch, > > but adding an extra warning print when test failed on older than > > kernel-v5.4. > > I like the idea of the warning. The only thing to consider is that the warning would also show up on all old kernels that don't even support lockdown and then don't have the file. So would you suggest this message > to be something like a tst_res(TWARN, ...) or TINFO or some other less > noisy way? > Thanks, but I did not suggest to show the warning in any system without the lockdown file. I mean if test getting FAIL on the no 'lockdown' file system, then could consider throwing a warning as a hint. And this could be achieved in the test case but not the library. For indicating the 'lockdown' file exist or no-exist, the 'TINFO' is enough.
diff --git a/include/tst_lockdown.h b/include/tst_lockdown.h new file mode 100644 index 000000000..8db26d943 --- /dev/null +++ b/include/tst_lockdown.h @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +#ifndef TST_LOCKDOWN_H +#define TST_LOCKDOWN_H + +void tst_lockdown_skip(void); + +#endif /* TST_LOCKDOWN_H */ diff --git a/include/tst_test.h b/include/tst_test.h index b84f7b9dd..b02de4597 100644 --- a/include/tst_test.h +++ b/include/tst_test.h @@ -40,6 +40,7 @@ #include "tst_hugepage.h" #include "tst_assert.h" #include "tst_cgroup.h" +#include "tst_lockdown.h" /* * Reports testcase result. diff --git a/lib/tst_lockdown.c b/lib/tst_lockdown.c new file mode 100644 index 000000000..d57a6bdf3 --- /dev/null +++ b/lib/tst_lockdown.c @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +#define TST_NO_DEFAULT_MAIN + +#include <stdio.h> +#include <stdlib.h> +#include <sys/mount.h> + +#include "tst_test.h" +#include "tst_safe_macros.h" +#include "tst_safe_stdio.h" +#include "tst_lockdown.h" + +void tst_lockdown_skip(void) +{ + char line[BUFSIZ]; + FILE *file; + + if (access("/sys/kernel/security/lockdown", F_OK) != 0) + return; + + file = SAFE_FOPEN("/sys/kernel/security/lockdown", "r"); + fgets(line, sizeof(line), file); + SAFE_FCLOSE(file); + + if (strstr(line, "[none]") == NULL) + tst_brk(TCONF, "Kernel is locked down, skip this test."); +}
Some syscalls are not available if the kernel is booted using the 'lockdown' feature. That can cause some tests to report fail, showing a message like: Lockdown: iopl01: iopl is restricted; see man kernel_lockdown.7 This patch adds a function that can be used by tests to check for this case, so tests can be skipped rather than reporting a test failure. Signed-off-by: Erico Nunes <ernunes@redhat.com> --- include/tst_lockdown.h | 8 ++++++++ include/tst_test.h | 1 + lib/tst_lockdown.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+) create mode 100644 include/tst_lockdown.h create mode 100644 lib/tst_lockdown.c