| Message ID | 20241217202525.1802109-1-song@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | ima: evm: Add kernel cmdline options to disable IMA/EVM | expand |
On 12/17/2024 12:25 PM, Song Liu wrote: > While reading and testing LSM code, I found IMA/EVM consume per inode > storage even when they are not in use. Add options to diable them in > kernel command line. The logic and syntax is mostly borrowed from an > old serious [1]. Why not omit ima and evm from the lsm= parameter? > > [1] https://lore.kernel.org/lkml/cover.1398259638.git.d.kasatkin@samsung.com/ > > Song Liu (2): > ima: Add kernel parameter to disable IMA > evm: Add kernel parameter to disable EVM > > security/integrity/evm/evm.h | 6 ++++++ > security/integrity/evm/evm_main.c | 22 ++++++++++++++-------- > security/integrity/evm/evm_secfs.c | 3 ++- > security/integrity/ima/ima_main.c | 13 +++++++++++++ > 4 files changed, 35 insertions(+), 9 deletions(-) > > -- > 2.43.5 >
On Tue, Dec 17, 2024 at 4:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 12/17/2024 12:25 PM, Song Liu wrote: > > While reading and testing LSM code, I found IMA/EVM consume per inode > > storage even when they are not in use. Add options to diable them in > > kernel command line. The logic and syntax is mostly borrowed from an > > old serious [1]. > > Why not omit ima and evm from the lsm= parameter? Exactly. Here is a link to the kernel documentation if anyone is interested (search for "lsm"): https://docs.kernel.org/admin-guide/kernel-parameters.html It is worth mentioning that this works for all the LSMs. > > [1] https://lore.kernel.org/lkml/cover.1398259638.git.d.kasatkin@samsung.com/ > > > > Song Liu (2): > > ima: Add kernel parameter to disable IMA > > evm: Add kernel parameter to disable EVM > > > > security/integrity/evm/evm.h | 6 ++++++ > > security/integrity/evm/evm_main.c | 22 ++++++++++++++-------- > > security/integrity/evm/evm_secfs.c | 3 ++- > > security/integrity/ima/ima_main.c | 13 +++++++++++++ > > 4 files changed, 35 insertions(+), 9 deletions(-) > > > > -- > > 2.43.5
> On Dec 17, 2024, at 1:29 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > > On 12/17/2024 12:25 PM, Song Liu wrote: >> While reading and testing LSM code, I found IMA/EVM consume per inode >> storage even when they are not in use. Add options to diable them in >> kernel command line. The logic and syntax is mostly borrowed from an >> old serious [1]. > > Why not omit ima and evm from the lsm= parameter? Both ima and evm have LSM_ORDER_LAST, so they are not controlled by lsm= parameter. But we can probably change this behavior in ordered_lsm_parse(), so that ima and evm are controlled by lsm=. Thanks, Song > >> >> [1] https://lore.kernel.org/lkml/cover.1398259638.git.d.kasatkin@samsung.com/ >> >> Song Liu (2): >> ima: Add kernel parameter to disable IMA >> evm: Add kernel parameter to disable EVM >> >> security/integrity/evm/evm.h | 6 ++++++ >> security/integrity/evm/evm_main.c | 22 ++++++++++++++-------- >> security/integrity/evm/evm_secfs.c | 3 ++- >> security/integrity/ima/ima_main.c | 13 +++++++++++++ >> 4 files changed, 35 insertions(+), 9 deletions(-) >> >> -- >> 2.43.5 >>
> On Dec 17, 2024, at 1:59 PM, Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Dec 17, 2024 at 4:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 12/17/2024 12:25 PM, Song Liu wrote: >>> While reading and testing LSM code, I found IMA/EVM consume per inode >>> storage even when they are not in use. Add options to diable them in >>> kernel command line. The logic and syntax is mostly borrowed from an >>> old serious [1]. >> >> Why not omit ima and evm from the lsm= parameter? > > Exactly. Here is a link to the kernel documentation if anyone is > interested (search for "lsm"): > > https://docs.kernel.org/admin-guide/kernel-parameters.html > > It is worth mentioning that this works for all the LSMs. I guess this is a bug that ima and evm do cannot be disabled by (not being add to) lsm= parameter? Thanks, Song > >>> [1] https://lore.kernel.org/lkml/cover.1398259638.git.d.kasatkin@samsung.com/ >>> >>> Song Liu (2): >>> ima: Add kernel parameter to disable IMA >>> evm: Add kernel parameter to disable EVM >>> >>> security/integrity/evm/evm.h | 6 ++++++ >>> security/integrity/evm/evm_main.c | 22 ++++++++++++++-------- >>> security/integrity/evm/evm_secfs.c | 3 ++- >>> security/integrity/ima/ima_main.c | 13 +++++++++++++ >>> 4 files changed, 35 insertions(+), 9 deletions(-) >>> >>> -- >>> 2.43.5 > > -- > paul-moore.com
> On Dec 17, 2024, at 2:04 PM, Song Liu <songliubraving@meta.com> wrote: > > > >> On Dec 17, 2024, at 1:59 PM, Paul Moore <paul@paul-moore.com> wrote: >> >> On Tue, Dec 17, 2024 at 4:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote: >>> On 12/17/2024 12:25 PM, Song Liu wrote: >>>> While reading and testing LSM code, I found IMA/EVM consume per inode >>>> storage even when they are not in use. Add options to diable them in >>>> kernel command line. The logic and syntax is mostly borrowed from an >>>> old serious [1]. >>> >>> Why not omit ima and evm from the lsm= parameter? >> >> Exactly. Here is a link to the kernel documentation if anyone is >> interested (search for "lsm"): >> >> https://docs.kernel.org/admin-guide/kernel-parameters.html >> >> It is worth mentioning that this works for all the LSMs. > > I guess this is a bug that ima and evm do cannot be disabled > by (not being add to) lsm= parameter? If we use lsm= to control ima and evm, we will need the following changes in ordered_lsm_parse(). We still need supporting logic in ima and evm side, so that ima and evm are only initialized when they are in lsm=. Does this sound the right way forward? Thanks, Song diff --git i/security/security.c w/security/security.c index 09664e09fec9..00271be3b0c1 100644 --- i/security/security.c +++ w/security/security.c @@ -365,6 +365,9 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) if (strcmp(lsm->name, name) == 0) { if (lsm->order == LSM_ORDER_MUTABLE) append_ordered_lsm(lsm, origin); + else if (lsm->order == LSM_ORDER_LAST) + set_enabled(lsm, true); + found = true; } } @@ -386,7 +389,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) /* LSM_ORDER_LAST is always last. */ for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) { - if (lsm->order == LSM_ORDER_LAST) + if (lsm->order == LSM_ORDER_LAST && is_enabled(lsm)) append_ordered_lsm(lsm, " last"); }
On Tue, Dec 17, 2024 at 5:47 PM Song Liu <songliubraving@meta.com> wrote: > > If we use lsm= to control ima and evm, we will need the following > changes in ordered_lsm_parse(). We still need supporting logic > in ima and evm side, so that ima and evm are only initialized > when they are in lsm=. > > Does this sound the right way forward? Have you tested it? What happens? There is value in going through the testing process, especially if you haven't played much with the LSM code. I'd also want to see a comment line in both places explaining why it is necessary to mark the LSM as enabled prior to actually adding it to @ordered_lsms. Something along the lines of only parsing the parameter once should be sufficient. > diff --git i/security/security.c w/security/security.c > index 09664e09fec9..00271be3b0c1 100644 > --- i/security/security.c > +++ w/security/security.c > @@ -365,6 +365,9 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) > if (strcmp(lsm->name, name) == 0) { > if (lsm->order == LSM_ORDER_MUTABLE) > append_ordered_lsm(lsm, origin); > + else if (lsm->order == LSM_ORDER_LAST) > + set_enabled(lsm, true); > + > found = true; > } > } > @@ -386,7 +389,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) > > /* LSM_ORDER_LAST is always last. */ > for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) { > - if (lsm->order == LSM_ORDER_LAST) > + if (lsm->order == LSM_ORDER_LAST && is_enabled(lsm)) > append_ordered_lsm(lsm, " last"); > }
> On Dec 17, 2024, at 3:16 PM, Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Dec 17, 2024 at 5:47 PM Song Liu <songliubraving@meta.com> wrote: >> >> If we use lsm= to control ima and evm, we will need the following >> changes in ordered_lsm_parse(). We still need supporting logic >> in ima and evm side, so that ima and evm are only initialized >> when they are in lsm=. >> >> Does this sound the right way forward? > > Have you tested it? What happens? There is value in going through > the testing process, especially if you haven't played much with the > LSM code. Yes, I tested both the original patches and the "lsm=xx" version. > > I'd also want to see a comment line in both places explaining why it > is necessary to mark the LSM as enabled prior to actually adding it to > @ordered_lsms. Something along the lines of only parsing the > parameter once should be sufficient. Please see below for the explanation. I will add different words in the actual comments so they make more sense as comments > >> diff --git i/security/security.c w/security/security.c >> index 09664e09fec9..00271be3b0c1 100644 >> --- i/security/security.c >> +++ w/security/security.c >> @@ -365,6 +365,9 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) >> if (strcmp(lsm->name, name) == 0) { >> if (lsm->order == LSM_ORDER_MUTABLE) >> append_ordered_lsm(lsm, origin); >> + else if (lsm->order == LSM_ORDER_LAST) >> + set_enabled(lsm, true); We need a flag here, saying we want to enable the lsm. We cannot do append_ordered_lsm() yet, otherwise, it will not be "last". >> + >> found = true; >> } >> } >> @@ -386,7 +389,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) >> >> /* LSM_ORDER_LAST is always last. */ >> for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) { >> - if (lsm->order == LSM_ORDER_LAST) >> + if (lsm->order == LSM_ORDER_LAST && is_enabled(lsm)) >> append_ordered_lsm(lsm, " last"); Before this change, lsm with order==LSM_ORDER_LAST is always considered enabled, which is a bug (if I understand you and Casey correctly). To fix this, we need a flag from above saying we actually want to enable it. I personally think it is fine to use set_enabled() to set the flag. But I don't have a strong preference, we can add a different flag. Does this make sense? Thanks, Song
> On Dec 17, 2024, at 3:33 PM, Song Liu <songliubraving@meta.com> wrote: [...] > >>> + >>> found = true; >>> } >>> } >>> @@ -386,7 +389,7 @@ static void __init ordered_lsm_parse(const char *order, const char *origin) >>> >>> /* LSM_ORDER_LAST is always last. */ >>> for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) { >>> - if (lsm->order == LSM_ORDER_LAST) >>> + if (lsm->order == LSM_ORDER_LAST && is_enabled(lsm)) >>> append_ordered_lsm(lsm, " last"); > > Before this change, lsm with order==LSM_ORDER_LAST is always considered > enabled, which is a bug (if I understand you and Casey correctly). According to commit 42994ee3cd7298b27698daa6848ed7168e72d056, LSMs with order LSM_ORDER_LAST is expected to be always enabled: "Similarly to LSM_ORDER_FIRST, LSMs with LSM_ORDER_LAST are always enabled and put at the end of the LSM list, if selected in the kernel configuration. " Roberto, it feels weird to have two "last and always on" LSMs (ima and evm) I guess this is not the expected behavior? At least, it appears to be a surprise for Paul and Casey. I will send patch that allow enable/disable ima and evm with lsm= cmdline. We can further discuss the topic with the patch. Thanks, Song