| Message ID | 1545092089.4206.5.camel@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: cleanup the match_token policy code | expand |
On Mon, Dec 17, 2018 at 4:14 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > Start the policy_tokens and the associated enumeration from zero, > simplifying the pt macro. I applied this directly, since I decided to just commit my own "don't use negative Opt_err" patch for the test_and_set_bit() cases, and they kind of go together. There's still a -1 in security/keys/encrypted-keys/encrypted.c, and there are also three cases of "Opt_error = -1" in the security layer. All of which look pointless and wrong, but not actively buggy, so I'll leave them alone. Linus
On Mon, Dec 17, 2018 at 04:36:54PM -0800, Linus Torvalds wrote: > On Mon, Dec 17, 2018 at 4:14 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > Start the policy_tokens and the associated enumeration from zero, > > simplifying the pt macro. > > I applied this directly, since I decided to just commit my own "don't > use negative Opt_err" patch for the test_and_set_bit() cases, and they > kind of go together. > > There's still a -1 in security/keys/encrypted-keys/encrypted.c, and > there are also three cases of "Opt_error = -1" in the security layer. > > All of which look pointless and wrong, but not actively buggy, so I'll > leave them alone. FWIW, that part of LSM shite is getting taken out and shot - LSM-related preps in the beginning of mount API series are, at the moment, at fs/btrfs/ctree.h | 4 - fs/btrfs/super.c | 82 ++------- fs/namespace.c | 9 +- fs/nfs/internal.h | 2 +- fs/nfs/super.c | 34 ++-- fs/super.c | 23 +-- include/linux/lsm_hooks.h | 17 +- include/linux/security.h | 82 ++------- security/security.c | 39 +++-- security/selinux/hooks.c | 798 ++++++++++++++++++++++++++++++++------------------------------------------------------ security/smack/smack_lsm.c | 359 +++++++++++++++------------------------ 11 files changed, 523 insertions(+), 926 deletions(-) and unlike the original variant there's not much touched in security/* further in the series... No match_token() uses left in there after this part.
On Tue, Dec 18, 2018 at 01:33:48AM +0000, Al Viro wrote: > On Mon, Dec 17, 2018 at 04:36:54PM -0800, Linus Torvalds wrote: > > On Mon, Dec 17, 2018 at 4:14 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > Start the policy_tokens and the associated enumeration from zero, > > > simplifying the pt macro. > > > > I applied this directly, since I decided to just commit my own "don't > > use negative Opt_err" patch for the test_and_set_bit() cases, and they > > kind of go together. > > > > There's still a -1 in security/keys/encrypted-keys/encrypted.c, and > > there are also three cases of "Opt_error = -1" in the security layer. > > > > All of which look pointless and wrong, but not actively buggy, so I'll > > leave them alone. > > FWIW, that part of LSM shite is getting taken out and shot - LSM-related > preps in the beginning of mount API series are, at the moment, at > fs/btrfs/ctree.h | 4 - > fs/btrfs/super.c | 82 ++------- > fs/namespace.c | 9 +- > fs/nfs/internal.h | 2 +- > fs/nfs/super.c | 34 ++-- > fs/super.c | 23 +-- > include/linux/lsm_hooks.h | 17 +- > include/linux/security.h | 82 ++------- > security/security.c | 39 +++-- > security/selinux/hooks.c | 798 ++++++++++++++++++++++++++++++++------------------------------------------------------ > security/smack/smack_lsm.c | 359 +++++++++++++++------------------------ > 11 files changed, 523 insertions(+), 926 deletions(-) > and unlike the original variant there's not much touched in security/* > further in the series... > > No match_token() uses left in there after this part. In LSM, that is...
On Tue, 2018-12-18 at 01:33 +0000, Al Viro wrote: > On Mon, Dec 17, 2018 at 04:36:54PM -0800, Linus Torvalds wrote: > > On Mon, Dec 17, 2018 at 4:14 PM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > Start the policy_tokens and the associated enumeration from zero, > > > simplifying the pt macro. > > > > I applied this directly, since I decided to just commit my own "don't > > use negative Opt_err" patch for the test_and_set_bit() cases, and they > > kind of go together. > > > > There's still a -1 in security/keys/encrypted-keys/encrypted.c, and > > there are also three cases of "Opt_error = -1" in the security layer. > > > > All of which look pointless and wrong, but not actively buggy, so I'll > > leave them alone. > > FWIW, that part of LSM shite is getting taken out and shot - LSM-related > preps in the beginning of mount API series are, at the moment, at > fs/btrfs/ctree.h | 4 - > fs/btrfs/super.c | 82 ++------- > fs/namespace.c | 9 +- > fs/nfs/internal.h | 2 +- > fs/nfs/super.c | 34 ++-- > fs/super.c | 23 +-- > include/linux/lsm_hooks.h | 17 +- > include/linux/security.h | 82 ++------- > security/security.c | 39 +++-- > security/selinux/hooks.c | 798 ++++++++++++++++++++++++++++++++------------------------------------------------------ > security/smack/smack_lsm.c | 359 +++++++++++++++------------------------ > 11 files changed, 523 insertions(+), 926 deletions(-) > and unlike the original variant there's not much touched in security/* > further in the series... > > No match_token() uses left in there after this part. Could you expand on commit 5b2ea6199614 ("selinux: switch away from match_token()") patch description. All that it says is "It's not a good fit, unfortunately, and the next step will make it even less so. Open-code what we need here." And there's even less for the equivalent Smack patch, which just says "same issue as with selinux...". Mimi
On Mon, Dec 17, 2018 at 10:00:07PM -0500, Mimi Zohar wrote: > Could you expand on commit 5b2ea6199614 ("selinux: switch away from > match_token()") patch description. All that it says is "It's not a > good fit, unfortunately, and the next step will make it even less so. > Open-code what we need here." And there's even less for the > equivalent Smack patch, which just says "same issue as with > selinux...". match_token() would require messing around with strsep() or something equivalent. It's not a regex; foo=%s has no idea that comma is in any way special, etc. As for the next commit... Killing the Cthulhu-awful mess in selinux_sb_eat_lsm_opts() (allocating two temproraries, concatenating (comma-separated) non-LSM options into one, concatenating (pipe-separated) dequoted LSM options into another, then splitting that another by '|' instances and figuring out which option each piece is, etc.) is a Good Thing(tm). And having to dance around the needs of match_token() adds extra headache, for no good reason.
On Tue, 2018-12-18 at 04:06 +0000, Al Viro wrote: > On Mon, Dec 17, 2018 at 10:00:07PM -0500, Mimi Zohar wrote: > > > Could you expand on commit 5b2ea6199614 ("selinux: switch away from > > match_token()") patch description. All that it says is "It's not a > > good fit, unfortunately, and the next step will make it even less so. > > Open-code what we need here." And there's even less for the > > equivalent Smack patch, which just says "same issue as with > > selinux...". > > match_token() would require messing around with strsep() or something > equivalent. It's not a regex; foo=%s has no idea that comma is in any > way special, etc. > > As for the next commit... Killing the Cthulhu-awful mess in > selinux_sb_eat_lsm_opts() (allocating two temproraries, concatenating > (comma-separated) non-LSM options into one, concatenating (pipe-separated) > dequoted LSM options into another, then splitting that another by '|' > instances and figuring out which option each piece is, etc.) > is a Good Thing(tm). And having to dance around the needs of > match_token() adds extra headache, for no good reason. Ok, so it is this particular combination of things, not the general usage of strsep or match_token that you're objecting to. So fixing the other match_token non-LSM instances is fine. To prevent the enumeration and the match_table from going out of sync, I was thinking about defining a macro to create the match_table_t: #define __policy_tokens_match(ENUM, str) {Opt_ ## ENUM, #str}, static const match_table_t policy_tokens = { __policy_tokens_id(__policy_tokens_match) }; and the enumeration: enum policy_tokens_id { __policy_tokens_id(__policy_tokens_enumify) }; Mimi
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index d17a23b5c91d..e89af72b98ed 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -665,9 +665,9 @@ void ima_update_policy(void) ima_update_policy_flag(); } +/* Keep the enumeration in sync with the policy_tokens! */ enum { - Opt_err = -1, - Opt_measure = 1, Opt_dont_measure, + Opt_measure, Opt_dont_measure, Opt_appraise, Opt_dont_appraise, Opt_audit, Opt_hash, Opt_dont_hash, Opt_obj_user, Opt_obj_role, Opt_obj_type, @@ -677,10 +677,10 @@ enum { Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt, Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt, Opt_appraise_type, Opt_permit_directio, - Opt_pcr + Opt_pcr, Opt_err }; -static match_table_t policy_tokens = { +static const match_table_t policy_tokens = { {Opt_measure, "measure"}, {Opt_dont_measure, "dont_measure"}, {Opt_appraise, "appraise"}, @@ -1188,7 +1188,7 @@ void ima_policy_stop(struct seq_file *m, void *v) { } -#define pt(token) policy_tokens[token + Opt_err].pattern +#define pt(token) policy_tokens[token].pattern #define mt(token) mask_tokens[token] /*
Start the policy_tokens and the associated enumeration from zero, simplifying the pt macro. Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- security/integrity/ima/ima_policy.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)