Currently security files use a mixture of octal and symbolic styles
for permissions.
Using octal and not symbolic permissions is preferred by many as more
readable.
see: https://lkml.org/lkml/2016/8/2/1945
Prefer the direct use of octal for permissions.
Done using:
$ git ls-files security | \
xargs ./scripts/checkpatch.pl -f --fix-inplace --types=symbolic_perms --strict
and some typing.
Before: $ git grep -P -w "0[0-7]{3,3}" security | wc -l
53
After: $ git grep -P -w "0[0-7]{3,3}" security | wc -l
136
Miscellanea:
o Whitespace neatening and line wrapping around these conversions.
o Remove now superfluous parentheses around direct use of 0600
Signed-off-by: Joe Perches <[email protected]>
---
security/apparmor/apparmorfs.c | 5 ++--
security/apparmor/lsm.c | 23 ++++++++---------
security/integrity/ima/ima.h | 4 +--
security/integrity/ima/ima_fs.c | 13 +++++-----
security/selinux/hooks.c | 4 +--
security/selinux/selinuxfs.c | 57 ++++++++++++++++++++---------------------
security/smack/smack_lsm.c | 6 ++---
security/smack/smackfs.c | 46 ++++++++++++++++-----------------
security/tomoyo/condition.c | 18 ++++++-------
9 files changed, 85 insertions(+), 91 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 949dd8a48164..c09dc0f3c3fe 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -2426,10 +2426,9 @@ static int aa_mk_null_file(struct dentry *parent)
}
inode->i_ino = get_next_ino();
- inode->i_mode = S_IFCHR | S_IRUGO | S_IWUGO;
+ inode->i_mode = S_IFCHR | 0666;
inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
- init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO,
- MKDEV(MEM_MAJOR, 3));
+ init_special_inode(inode, S_IFCHR | 0666, MKDEV(MEM_MAJOR, 3));
d_instantiate(dentry, inode);
aa_null.dentry = dget(dentry);
aa_null.mnt = mntget(mount);
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index fbb08bc78bee..6759a70918de 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1255,45 +1255,42 @@ static int param_get_mode(char *buffer, const struct kernel_param *kp);
/* AppArmor global enforcement switch - complain, enforce, kill */
enum profile_mode aa_g_profile_mode = APPARMOR_ENFORCE;
module_param_call(mode, param_set_mode, param_get_mode,
- &aa_g_profile_mode, S_IRUSR | S_IWUSR);
+ &aa_g_profile_mode, 0600);
/* whether policy verification hashing is enabled */
bool aa_g_hash_policy = IS_ENABLED(CONFIG_SECURITY_APPARMOR_HASH_DEFAULT);
#ifdef CONFIG_SECURITY_APPARMOR_HASH
-module_param_named(hash_policy, aa_g_hash_policy, aabool, S_IRUSR | S_IWUSR);
+module_param_named(hash_policy, aa_g_hash_policy, aabool, 0600);
#endif
/* Debug mode */
bool aa_g_debug = IS_ENABLED(CONFIG_SECURITY_APPARMOR_DEBUG_MESSAGES);
-module_param_named(debug, aa_g_debug, aabool, S_IRUSR | S_IWUSR);
+module_param_named(debug, aa_g_debug, aabool, 0600);
/* Audit mode */
enum audit_mode aa_g_audit;
-module_param_call(audit, param_set_audit, param_get_audit,
- &aa_g_audit, S_IRUSR | S_IWUSR);
+module_param_call(audit, param_set_audit, param_get_audit, &aa_g_audit, 0600);
/* Determines if audit header is included in audited messages. This
* provides more context if the audit daemon is not running
*/
bool aa_g_audit_header = true;
-module_param_named(audit_header, aa_g_audit_header, aabool,
- S_IRUSR | S_IWUSR);
+module_param_named(audit_header, aa_g_audit_header, aabool, 0600);
/* lock out loading/removal of policy
* TODO: add in at boot loading of policy, which is the only way to
* load policy, if lock_policy is set
*/
bool aa_g_lock_policy;
-module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy,
- S_IRUSR | S_IWUSR);
+module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy, 0600);
/* Syscall logging mode */
bool aa_g_logsyscall;
-module_param_named(logsyscall, aa_g_logsyscall, aabool, S_IRUSR | S_IWUSR);
+module_param_named(logsyscall, aa_g_logsyscall, aabool, 0600);
/* Maximum pathname length before accesses will start getting rejected */
unsigned int aa_g_path_max = 2 * PATH_MAX;
-module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
+module_param_named(path_max, aa_g_path_max, aauint, 0400);
/* Determines how paranoid loading of policy is and how much verification
* on the loaded policy is done.
@@ -1301,11 +1298,11 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
* that none root users (user namespaces) can load policy.
*/
bool aa_g_paranoid_load = true;
-module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
+module_param_named(paranoid_load, aa_g_paranoid_load, aabool, 0444);
/* Boot time disable flag */
static bool apparmor_enabled = CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE;
-module_param_named(enabled, apparmor_enabled, bool, S_IRUGO);
+module_param_named(enabled, apparmor_enabled, bool, 0444);
static int __init apparmor_enabled_setup(char *str)
{
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 354bb5716ce3..3f7707b8aaa7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -314,9 +314,9 @@ static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
#endif /* CONFIG_IMA_LSM_RULES */
#ifdef CONFIG_IMA_READ_POLICY
-#define POLICY_FILE_FLAGS (S_IWUSR | S_IRUSR)
+#define POLICY_FILE_FLAGS 0600
#else
-#define POLICY_FILE_FLAGS S_IWUSR
+#define POLICY_FILE_FLAGS 0200
#endif /* CONFIG_IMA_READ_POLICY */
#endif /* __LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index ae9d5c766a3c..81700df83f51 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -439,7 +439,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
#elif defined(CONFIG_IMA_WRITE_POLICY)
clear_bit(IMA_FS_BUSY, &ima_fs_flags);
#elif defined(CONFIG_IMA_READ_POLICY)
- inode->i_mode &= ~S_IWUSR;
+ inode->i_mode &= ~0200;
#endif
return 0;
}
@@ -465,28 +465,29 @@ int __init ima_fs_init(void)
binary_runtime_measurements =
securityfs_create_file("binary_runtime_measurements",
- S_IRUSR | S_IRGRP, ima_dir, NULL,
+ 0440, ima_dir, NULL,
&ima_measurements_ops);
if (IS_ERR(binary_runtime_measurements))
goto out;
ascii_runtime_measurements =
securityfs_create_file("ascii_runtime_measurements",
- S_IRUSR | S_IRGRP, ima_dir, NULL,
+ 0440, ima_dir, NULL,
&ima_ascii_measurements_ops);
if (IS_ERR(ascii_runtime_measurements))
goto out;
runtime_measurements_count =
securityfs_create_file("runtime_measurements_count",
- S_IRUSR | S_IRGRP, ima_dir, NULL,
+ 0440, ima_dir, NULL,
&ima_measurements_count_ops);
if (IS_ERR(runtime_measurements_count))
goto out;
violations =
- securityfs_create_file("violations", S_IRUSR | S_IRGRP,
- ima_dir, NULL, &ima_htable_violations_ops);
+ securityfs_create_file("violations",
+ 0440, ima_dir, NULL,
+ &ima_htable_violations_ops);
if (IS_ERR(violations))
goto out;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a85fac3345df..8ae043be8782 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6336,9 +6336,9 @@ static int selinux_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
u32 av = 0;
av = 0;
- if (flag & S_IRUGO)
+ if (flag & 0444)
av |= IPC__UNIX_READ;
- if (flag & S_IWUGO)
+ if (flag & 0222)
av |= IPC__UNIX_WRITE;
if (av == 0)
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index f3d374d2ca04..bfecac19ba92 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1376,7 +1376,7 @@ static int sel_make_bools(struct selinux_fs_info *fsi)
goto out;
ret = -ENOMEM;
- inode = sel_make_inode(dir->d_sb, S_IFREG | S_IRUGO | S_IWUSR);
+ inode = sel_make_inode(dir->d_sb, S_IFREG | 0644);
if (!inode)
goto out;
@@ -1582,10 +1582,10 @@ static int sel_make_avc_files(struct dentry *dir)
int i;
static const struct tree_descr files[] = {
{ "cache_threshold",
- &sel_avc_cache_threshold_ops, S_IRUGO|S_IWUSR },
- { "hash_stats", &sel_avc_hash_stats_ops, S_IRUGO },
+ &sel_avc_cache_threshold_ops, 0644 },
+ { "hash_stats", &sel_avc_hash_stats_ops, 0444 },
#ifdef CONFIG_SECURITY_SELINUX_AVC_STATS
- { "cache_stats", &sel_avc_cache_stats_ops, S_IRUGO },
+ { "cache_stats", &sel_avc_cache_stats_ops, 0444 },
#endif
};
@@ -1643,7 +1643,7 @@ static int sel_make_initcon_files(struct dentry *dir)
if (!dentry)
return -ENOMEM;
- inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
+ inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
if (!inode)
return -ENOMEM;
@@ -1744,7 +1744,7 @@ static int sel_make_perm_files(char *objclass, int classvalue,
goto out;
rc = -ENOMEM;
- inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
+ inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
if (!inode)
goto out;
@@ -1774,7 +1774,7 @@ static int sel_make_class_dir_entries(char *classname, int index,
if (!dentry)
return -ENOMEM;
- inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
+ inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
if (!inode)
return -ENOMEM;
@@ -1870,7 +1870,7 @@ static struct dentry *sel_make_dir(struct dentry *dir, const char *name,
if (!dentry)
return ERR_PTR(-ENOMEM);
- inode = sel_make_inode(dir->d_sb, S_IFDIR | S_IRUGO | S_IXUGO);
+ inode = sel_make_inode(dir->d_sb, S_IFDIR | 0555);
if (!inode) {
dput(dentry);
return ERR_PTR(-ENOMEM);
@@ -1899,25 +1899,24 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
struct inode_security_struct *isec;
static const struct tree_descr selinux_files[] = {
- [SEL_LOAD] = {"load", &sel_load_ops, S_IRUSR|S_IWUSR},
- [SEL_ENFORCE] = {"enforce", &sel_enforce_ops, S_IRUGO|S_IWUSR},
- [SEL_CONTEXT] = {"context", &transaction_ops, S_IRUGO|S_IWUGO},
- [SEL_ACCESS] = {"access", &transaction_ops, S_IRUGO|S_IWUGO},
- [SEL_CREATE] = {"create", &transaction_ops, S_IRUGO|S_IWUGO},
- [SEL_RELABEL] = {"relabel", &transaction_ops, S_IRUGO|S_IWUGO},
- [SEL_USER] = {"user", &transaction_ops, S_IRUGO|S_IWUGO},
- [SEL_POLICYVERS] = {"policyvers", &sel_policyvers_ops, S_IRUGO},
- [SEL_COMMIT_BOOLS] = {"commit_pending_bools", &sel_commit_bools_ops, S_IWUSR},
- [SEL_MLS] = {"mls", &sel_mls_ops, S_IRUGO},
- [SEL_DISABLE] = {"disable", &sel_disable_ops, S_IWUSR},
- [SEL_MEMBER] = {"member", &transaction_ops, S_IRUGO|S_IWUGO},
- [SEL_CHECKREQPROT] = {"checkreqprot", &sel_checkreqprot_ops, S_IRUGO|S_IWUSR},
- [SEL_REJECT_UNKNOWN] = {"reject_unknown", &sel_handle_unknown_ops, S_IRUGO},
- [SEL_DENY_UNKNOWN] = {"deny_unknown", &sel_handle_unknown_ops, S_IRUGO},
- [SEL_STATUS] = {"status", &sel_handle_status_ops, S_IRUGO},
- [SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUGO},
- [SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops,
- S_IWUGO},
+ [SEL_LOAD] = {"load", &sel_load_ops, 0600},
+ [SEL_ENFORCE] = {"enforce", &sel_enforce_ops, 0644},
+ [SEL_CONTEXT] = {"context", &transaction_ops, 0666},
+ [SEL_ACCESS] = {"access", &transaction_ops, 0666},
+ [SEL_CREATE] = {"create", &transaction_ops, 0666},
+ [SEL_RELABEL] = {"relabel", &transaction_ops, 0666},
+ [SEL_USER] = {"user", &transaction_ops, 0666},
+ [SEL_POLICYVERS] = {"policyvers", &sel_policyvers_ops, 0444},
+ [SEL_COMMIT_BOOLS] = {"commit_pending_bools", &sel_commit_bools_ops, 0200},
+ [SEL_MLS] = {"mls", &sel_mls_ops, 0444},
+ [SEL_DISABLE] = {"disable", &sel_disable_ops, 0200},
+ [SEL_MEMBER] = {"member", &transaction_ops, 0666},
+ [SEL_CHECKREQPROT] = {"checkreqprot", &sel_checkreqprot_ops, 0644},
+ [SEL_REJECT_UNKNOWN] = {"reject_unknown", &sel_handle_unknown_ops, 0444},
+ [SEL_DENY_UNKNOWN] = {"deny_unknown", &sel_handle_unknown_ops, 0444},
+ [SEL_STATUS] = {"status", &sel_handle_status_ops, 0444},
+ [SEL_POLICY] = {"policy", &sel_policy_ops, 0444},
+ [SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops, 0222},
/* last one */ {""}
};
@@ -1943,7 +1942,7 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
goto err;
ret = -ENOMEM;
- inode = sel_make_inode(sb, S_IFCHR | S_IRUGO | S_IWUGO);
+ inode = sel_make_inode(sb, S_IFCHR | 0666);
if (!inode)
goto err;
@@ -1953,7 +1952,7 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
isec->sclass = SECCLASS_CHR_FILE;
isec->initialized = LABEL_INITIALIZED;
- init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO, MKDEV(MEM_MAJOR, 3));
+ init_special_inode(inode, S_IFCHR | 0666, MKDEV(MEM_MAJOR, 3));
d_add(dentry, inode);
dentry = sel_make_dir(sb->s_root, "avc", &fsi->last_ino);
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index dcb976f98df2..8953440c6559 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2945,11 +2945,11 @@ static int smack_flags_to_may(int flags)
{
int may = 0;
- if (flags & S_IRUGO)
+ if (flags & 0444)
may |= MAY_READ;
- if (flags & S_IWUGO)
+ if (flags & 0222)
may |= MAY_WRITE;
- if (flags & S_IXUGO)
+ if (flags & 0111)
may |= MAY_EXEC;
return may;
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index f6482e53d55a..270cd3a308f0 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -2857,55 +2857,53 @@ static int smk_fill_super(struct super_block *sb, void *data, int silent)
static const struct tree_descr smack_files[] = {
[SMK_LOAD] = {
- "load", &smk_load_ops, S_IRUGO|S_IWUSR},
+ "load", &smk_load_ops, 0644},
[SMK_CIPSO] = {
- "cipso", &smk_cipso_ops, S_IRUGO|S_IWUSR},
+ "cipso", &smk_cipso_ops, 0644},
[SMK_DOI] = {
- "doi", &smk_doi_ops, S_IRUGO|S_IWUSR},
+ "doi", &smk_doi_ops, 0644},
[SMK_DIRECT] = {
- "direct", &smk_direct_ops, S_IRUGO|S_IWUSR},
+ "direct", &smk_direct_ops, 0644},
[SMK_AMBIENT] = {
- "ambient", &smk_ambient_ops, S_IRUGO|S_IWUSR},
+ "ambient", &smk_ambient_ops, 0644},
[SMK_NET4ADDR] = {
- "netlabel", &smk_net4addr_ops, S_IRUGO|S_IWUSR},
+ "netlabel", &smk_net4addr_ops, 0644},
[SMK_ONLYCAP] = {
- "onlycap", &smk_onlycap_ops, S_IRUGO|S_IWUSR},
+ "onlycap", &smk_onlycap_ops, 0644},
[SMK_LOGGING] = {
- "logging", &smk_logging_ops, S_IRUGO|S_IWUSR},
+ "logging", &smk_logging_ops, 0644},
[SMK_LOAD_SELF] = {
- "load-self", &smk_load_self_ops, S_IRUGO|S_IWUGO},
+ "load-self", &smk_load_self_ops, 0666},
[SMK_ACCESSES] = {
- "access", &smk_access_ops, S_IRUGO|S_IWUGO},
+ "access", &smk_access_ops, 0666},
[SMK_MAPPED] = {
- "mapped", &smk_mapped_ops, S_IRUGO|S_IWUSR},
+ "mapped", &smk_mapped_ops, 0644},
[SMK_LOAD2] = {
- "load2", &smk_load2_ops, S_IRUGO|S_IWUSR},
+ "load2", &smk_load2_ops, 0644},
[SMK_LOAD_SELF2] = {
- "load-self2", &smk_load_self2_ops, S_IRUGO|S_IWUGO},
+ "load-self2", &smk_load_self2_ops, 0666},
[SMK_ACCESS2] = {
- "access2", &smk_access2_ops, S_IRUGO|S_IWUGO},
+ "access2", &smk_access2_ops, 0666},
[SMK_CIPSO2] = {
- "cipso2", &smk_cipso2_ops, S_IRUGO|S_IWUSR},
+ "cipso2", &smk_cipso2_ops, 0644},
[SMK_REVOKE_SUBJ] = {
- "revoke-subject", &smk_revoke_subj_ops,
- S_IRUGO|S_IWUSR},
+ "revoke-subject", &smk_revoke_subj_ops, 0644},
[SMK_CHANGE_RULE] = {
- "change-rule", &smk_change_rule_ops, S_IRUGO|S_IWUSR},
+ "change-rule", &smk_change_rule_ops, 0644},
[SMK_SYSLOG] = {
- "syslog", &smk_syslog_ops, S_IRUGO|S_IWUSR},
+ "syslog", &smk_syslog_ops, 0644},
[SMK_PTRACE] = {
- "ptrace", &smk_ptrace_ops, S_IRUGO|S_IWUSR},
+ "ptrace", &smk_ptrace_ops, 0644},
#ifdef CONFIG_SECURITY_SMACK_BRINGUP
[SMK_UNCONFINED] = {
- "unconfined", &smk_unconfined_ops, S_IRUGO|S_IWUSR},
+ "unconfined", &smk_unconfined_ops, 0644},
#endif
#if IS_ENABLED(CONFIG_IPV6)
[SMK_NET6ADDR] = {
- "ipv6host", &smk_net6addr_ops, S_IRUGO|S_IWUSR},
+ "ipv6host", &smk_net6addr_ops, 0644},
#endif /* CONFIG_IPV6 */
[SMK_RELABEL_SELF] = {
- "relabel-self", &smk_relabel_self_ops,
- S_IRUGO|S_IWUGO},
+ "relabel-self", &smk_relabel_self_ops, 0666},
/* last one */
{""}
};
diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
index 8d0e1b9c9c57..2069f5912469 100644
--- a/security/tomoyo/condition.c
+++ b/security/tomoyo/condition.c
@@ -874,31 +874,31 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
value = S_ISVTX;
break;
case TOMOYO_MODE_OWNER_READ:
- value = S_IRUSR;
+ value = 0400;
break;
case TOMOYO_MODE_OWNER_WRITE:
- value = S_IWUSR;
+ value = 0200;
break;
case TOMOYO_MODE_OWNER_EXECUTE:
- value = S_IXUSR;
+ value = 0100;
break;
case TOMOYO_MODE_GROUP_READ:
- value = S_IRGRP;
+ value = 0040;
break;
case TOMOYO_MODE_GROUP_WRITE:
- value = S_IWGRP;
+ value = 0020;
break;
case TOMOYO_MODE_GROUP_EXECUTE:
- value = S_IXGRP;
+ value = 0010;
break;
case TOMOYO_MODE_OTHERS_READ:
- value = S_IROTH;
+ value = 0004;
break;
case TOMOYO_MODE_OTHERS_WRITE:
- value = S_IWOTH;
+ value = 0002;
break;
case TOMOYO_MODE_OTHERS_EXECUTE:
- value = S_IXOTH;
+ value = 0001;
break;
case TOMOYO_EXEC_ARGC:
if (!bprm)
--
2.15.0
On 6/11/2018 12:01 PM, Joe Perches wrote:
> Currently security files use a mixture of octal and symbolic styles
> for permissions.
>
> Using octal and not symbolic permissions is preferred by many as more
> readable.
>
> see: https://lkml.org/lkml/2016/8/2/1945
>
> Prefer the direct use of octal for permissions.
>
> Done using:
>
> $ git ls-files security | \
> xargs ./scripts/checkpatch.pl -f --fix-inplace --types=symbolic_perms --strict
>
> and some typing.
>
> Before: $ git grep -P -w "0[0-7]{3,3}" security | wc -l
> 53
> After: $ git grep -P -w "0[0-7]{3,3}" security | wc -l
> 136
>
> Miscellanea:
>
> o Whitespace neatening and line wrapping around these conversions.
> o Remove now superfluous parentheses around direct use of 0600
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> security/apparmor/apparmorfs.c | 5 ++--
> security/apparmor/lsm.c | 23 ++++++++---------
> security/integrity/ima/ima.h | 4 +--
> security/integrity/ima/ima_fs.c | 13 +++++-----
> security/selinux/hooks.c | 4 +--
> security/selinux/selinuxfs.c | 57 ++++++++++++++++++++---------------------
> security/smack/smack_lsm.c | 6 ++---
> security/smack/smackfs.c | 46 ++++++++++++++++-----------------
> security/tomoyo/condition.c | 18 ++++++-------
> 9 files changed, 85 insertions(+), 91 deletions(-)
If you want to break this up by security module I would take
the Smack part as soon as James does the tree update. If James
wants to take the whole thing at once you can add my:
Acked-by: Casey Schaufler <[email protected]>
for the Smack changes.
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 949dd8a48164..c09dc0f3c3fe 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -2426,10 +2426,9 @@ static int aa_mk_null_file(struct dentry *parent)
> }
>
> inode->i_ino = get_next_ino();
> - inode->i_mode = S_IFCHR | S_IRUGO | S_IWUGO;
> + inode->i_mode = S_IFCHR | 0666;
> inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> - init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO,
> - MKDEV(MEM_MAJOR, 3));
> + init_special_inode(inode, S_IFCHR | 0666, MKDEV(MEM_MAJOR, 3));
> d_instantiate(dentry, inode);
> aa_null.dentry = dget(dentry);
> aa_null.mnt = mntget(mount);
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index fbb08bc78bee..6759a70918de 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1255,45 +1255,42 @@ static int param_get_mode(char *buffer, const struct kernel_param *kp);
> /* AppArmor global enforcement switch - complain, enforce, kill */
> enum profile_mode aa_g_profile_mode = APPARMOR_ENFORCE;
> module_param_call(mode, param_set_mode, param_get_mode,
> - &aa_g_profile_mode, S_IRUSR | S_IWUSR);
> + &aa_g_profile_mode, 0600);
>
> /* whether policy verification hashing is enabled */
> bool aa_g_hash_policy = IS_ENABLED(CONFIG_SECURITY_APPARMOR_HASH_DEFAULT);
> #ifdef CONFIG_SECURITY_APPARMOR_HASH
> -module_param_named(hash_policy, aa_g_hash_policy, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(hash_policy, aa_g_hash_policy, aabool, 0600);
> #endif
>
> /* Debug mode */
> bool aa_g_debug = IS_ENABLED(CONFIG_SECURITY_APPARMOR_DEBUG_MESSAGES);
> -module_param_named(debug, aa_g_debug, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(debug, aa_g_debug, aabool, 0600);
>
> /* Audit mode */
> enum audit_mode aa_g_audit;
> -module_param_call(audit, param_set_audit, param_get_audit,
> - &aa_g_audit, S_IRUSR | S_IWUSR);
> +module_param_call(audit, param_set_audit, param_get_audit, &aa_g_audit, 0600);
>
> /* Determines if audit header is included in audited messages. This
> * provides more context if the audit daemon is not running
> */
> bool aa_g_audit_header = true;
> -module_param_named(audit_header, aa_g_audit_header, aabool,
> - S_IRUSR | S_IWUSR);
> +module_param_named(audit_header, aa_g_audit_header, aabool, 0600);
>
> /* lock out loading/removal of policy
> * TODO: add in at boot loading of policy, which is the only way to
> * load policy, if lock_policy is set
> */
> bool aa_g_lock_policy;
> -module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy,
> - S_IRUSR | S_IWUSR);
> +module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy, 0600);
>
> /* Syscall logging mode */
> bool aa_g_logsyscall;
> -module_param_named(logsyscall, aa_g_logsyscall, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(logsyscall, aa_g_logsyscall, aabool, 0600);
>
> /* Maximum pathname length before accesses will start getting rejected */
> unsigned int aa_g_path_max = 2 * PATH_MAX;
> -module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
> +module_param_named(path_max, aa_g_path_max, aauint, 0400);
>
> /* Determines how paranoid loading of policy is and how much verification
> * on the loaded policy is done.
> @@ -1301,11 +1298,11 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
> * that none root users (user namespaces) can load policy.
> */
> bool aa_g_paranoid_load = true;
> -module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
> +module_param_named(paranoid_load, aa_g_paranoid_load, aabool, 0444);
>
> /* Boot time disable flag */
> static bool apparmor_enabled = CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE;
> -module_param_named(enabled, apparmor_enabled, bool, S_IRUGO);
> +module_param_named(enabled, apparmor_enabled, bool, 0444);
>
> static int __init apparmor_enabled_setup(char *str)
> {
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 354bb5716ce3..3f7707b8aaa7 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -314,9 +314,9 @@ static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> #endif /* CONFIG_IMA_LSM_RULES */
>
> #ifdef CONFIG_IMA_READ_POLICY
> -#define POLICY_FILE_FLAGS (S_IWUSR | S_IRUSR)
> +#define POLICY_FILE_FLAGS 0600
> #else
> -#define POLICY_FILE_FLAGS S_IWUSR
> +#define POLICY_FILE_FLAGS 0200
> #endif /* CONFIG_IMA_READ_POLICY */
>
> #endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index ae9d5c766a3c..81700df83f51 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -439,7 +439,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
> #elif defined(CONFIG_IMA_WRITE_POLICY)
> clear_bit(IMA_FS_BUSY, &ima_fs_flags);
> #elif defined(CONFIG_IMA_READ_POLICY)
> - inode->i_mode &= ~S_IWUSR;
> + inode->i_mode &= ~0200;
> #endif
> return 0;
> }
> @@ -465,28 +465,29 @@ int __init ima_fs_init(void)
>
> binary_runtime_measurements =
> securityfs_create_file("binary_runtime_measurements",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> + 0440, ima_dir, NULL,
> &ima_measurements_ops);
> if (IS_ERR(binary_runtime_measurements))
> goto out;
>
> ascii_runtime_measurements =
> securityfs_create_file("ascii_runtime_measurements",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> + 0440, ima_dir, NULL,
> &ima_ascii_measurements_ops);
> if (IS_ERR(ascii_runtime_measurements))
> goto out;
>
> runtime_measurements_count =
> securityfs_create_file("runtime_measurements_count",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> + 0440, ima_dir, NULL,
> &ima_measurements_count_ops);
> if (IS_ERR(runtime_measurements_count))
> goto out;
>
> violations =
> - securityfs_create_file("violations", S_IRUSR | S_IRGRP,
> - ima_dir, NULL, &ima_htable_violations_ops);
> + securityfs_create_file("violations",
> + 0440, ima_dir, NULL,
> + &ima_htable_violations_ops);
> if (IS_ERR(violations))
> goto out;
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a85fac3345df..8ae043be8782 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6336,9 +6336,9 @@ static int selinux_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
> u32 av = 0;
>
> av = 0;
> - if (flag & S_IRUGO)
> + if (flag & 0444)
> av |= IPC__UNIX_READ;
> - if (flag & S_IWUGO)
> + if (flag & 0222)
> av |= IPC__UNIX_WRITE;
>
> if (av == 0)
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index f3d374d2ca04..bfecac19ba92 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1376,7 +1376,7 @@ static int sel_make_bools(struct selinux_fs_info *fsi)
> goto out;
>
> ret = -ENOMEM;
> - inode = sel_make_inode(dir->d_sb, S_IFREG | S_IRUGO | S_IWUSR);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0644);
> if (!inode)
> goto out;
>
> @@ -1582,10 +1582,10 @@ static int sel_make_avc_files(struct dentry *dir)
> int i;
> static const struct tree_descr files[] = {
> { "cache_threshold",
> - &sel_avc_cache_threshold_ops, S_IRUGO|S_IWUSR },
> - { "hash_stats", &sel_avc_hash_stats_ops, S_IRUGO },
> + &sel_avc_cache_threshold_ops, 0644 },
> + { "hash_stats", &sel_avc_hash_stats_ops, 0444 },
> #ifdef CONFIG_SECURITY_SELINUX_AVC_STATS
> - { "cache_stats", &sel_avc_cache_stats_ops, S_IRUGO },
> + { "cache_stats", &sel_avc_cache_stats_ops, 0444 },
> #endif
> };
>
> @@ -1643,7 +1643,7 @@ static int sel_make_initcon_files(struct dentry *dir)
> if (!dentry)
> return -ENOMEM;
>
> - inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
> if (!inode)
> return -ENOMEM;
>
> @@ -1744,7 +1744,7 @@ static int sel_make_perm_files(char *objclass, int classvalue,
> goto out;
>
> rc = -ENOMEM;
> - inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
> if (!inode)
> goto out;
>
> @@ -1774,7 +1774,7 @@ static int sel_make_class_dir_entries(char *classname, int index,
> if (!dentry)
> return -ENOMEM;
>
> - inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
> if (!inode)
> return -ENOMEM;
>
> @@ -1870,7 +1870,7 @@ static struct dentry *sel_make_dir(struct dentry *dir, const char *name,
> if (!dentry)
> return ERR_PTR(-ENOMEM);
>
> - inode = sel_make_inode(dir->d_sb, S_IFDIR | S_IRUGO | S_IXUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFDIR | 0555);
> if (!inode) {
> dput(dentry);
> return ERR_PTR(-ENOMEM);
> @@ -1899,25 +1899,24 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
> struct inode_security_struct *isec;
>
> static const struct tree_descr selinux_files[] = {
> - [SEL_LOAD] = {"load", &sel_load_ops, S_IRUSR|S_IWUSR},
> - [SEL_ENFORCE] = {"enforce", &sel_enforce_ops, S_IRUGO|S_IWUSR},
> - [SEL_CONTEXT] = {"context", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_ACCESS] = {"access", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_CREATE] = {"create", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_RELABEL] = {"relabel", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_USER] = {"user", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_POLICYVERS] = {"policyvers", &sel_policyvers_ops, S_IRUGO},
> - [SEL_COMMIT_BOOLS] = {"commit_pending_bools", &sel_commit_bools_ops, S_IWUSR},
> - [SEL_MLS] = {"mls", &sel_mls_ops, S_IRUGO},
> - [SEL_DISABLE] = {"disable", &sel_disable_ops, S_IWUSR},
> - [SEL_MEMBER] = {"member", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_CHECKREQPROT] = {"checkreqprot", &sel_checkreqprot_ops, S_IRUGO|S_IWUSR},
> - [SEL_REJECT_UNKNOWN] = {"reject_unknown", &sel_handle_unknown_ops, S_IRUGO},
> - [SEL_DENY_UNKNOWN] = {"deny_unknown", &sel_handle_unknown_ops, S_IRUGO},
> - [SEL_STATUS] = {"status", &sel_handle_status_ops, S_IRUGO},
> - [SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUGO},
> - [SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops,
> - S_IWUGO},
> + [SEL_LOAD] = {"load", &sel_load_ops, 0600},
> + [SEL_ENFORCE] = {"enforce", &sel_enforce_ops, 0644},
> + [SEL_CONTEXT] = {"context", &transaction_ops, 0666},
> + [SEL_ACCESS] = {"access", &transaction_ops, 0666},
> + [SEL_CREATE] = {"create", &transaction_ops, 0666},
> + [SEL_RELABEL] = {"relabel", &transaction_ops, 0666},
> + [SEL_USER] = {"user", &transaction_ops, 0666},
> + [SEL_POLICYVERS] = {"policyvers", &sel_policyvers_ops, 0444},
> + [SEL_COMMIT_BOOLS] = {"commit_pending_bools", &sel_commit_bools_ops, 0200},
> + [SEL_MLS] = {"mls", &sel_mls_ops, 0444},
> + [SEL_DISABLE] = {"disable", &sel_disable_ops, 0200},
> + [SEL_MEMBER] = {"member", &transaction_ops, 0666},
> + [SEL_CHECKREQPROT] = {"checkreqprot", &sel_checkreqprot_ops, 0644},
> + [SEL_REJECT_UNKNOWN] = {"reject_unknown", &sel_handle_unknown_ops, 0444},
> + [SEL_DENY_UNKNOWN] = {"deny_unknown", &sel_handle_unknown_ops, 0444},
> + [SEL_STATUS] = {"status", &sel_handle_status_ops, 0444},
> + [SEL_POLICY] = {"policy", &sel_policy_ops, 0444},
> + [SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops, 0222},
> /* last one */ {""}
> };
>
> @@ -1943,7 +1942,7 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
> goto err;
>
> ret = -ENOMEM;
> - inode = sel_make_inode(sb, S_IFCHR | S_IRUGO | S_IWUGO);
> + inode = sel_make_inode(sb, S_IFCHR | 0666);
> if (!inode)
> goto err;
>
> @@ -1953,7 +1952,7 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
> isec->sclass = SECCLASS_CHR_FILE;
> isec->initialized = LABEL_INITIALIZED;
>
> - init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO, MKDEV(MEM_MAJOR, 3));
> + init_special_inode(inode, S_IFCHR | 0666, MKDEV(MEM_MAJOR, 3));
> d_add(dentry, inode);
>
> dentry = sel_make_dir(sb->s_root, "avc", &fsi->last_ino);
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index dcb976f98df2..8953440c6559 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2945,11 +2945,11 @@ static int smack_flags_to_may(int flags)
> {
> int may = 0;
>
> - if (flags & S_IRUGO)
> + if (flags & 0444)
> may |= MAY_READ;
> - if (flags & S_IWUGO)
> + if (flags & 0222)
> may |= MAY_WRITE;
> - if (flags & S_IXUGO)
> + if (flags & 0111)
> may |= MAY_EXEC;
>
> return may;
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index f6482e53d55a..270cd3a308f0 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -2857,55 +2857,53 @@ static int smk_fill_super(struct super_block *sb, void *data, int silent)
>
> static const struct tree_descr smack_files[] = {
> [SMK_LOAD] = {
> - "load", &smk_load_ops, S_IRUGO|S_IWUSR},
> + "load", &smk_load_ops, 0644},
> [SMK_CIPSO] = {
> - "cipso", &smk_cipso_ops, S_IRUGO|S_IWUSR},
> + "cipso", &smk_cipso_ops, 0644},
> [SMK_DOI] = {
> - "doi", &smk_doi_ops, S_IRUGO|S_IWUSR},
> + "doi", &smk_doi_ops, 0644},
> [SMK_DIRECT] = {
> - "direct", &smk_direct_ops, S_IRUGO|S_IWUSR},
> + "direct", &smk_direct_ops, 0644},
> [SMK_AMBIENT] = {
> - "ambient", &smk_ambient_ops, S_IRUGO|S_IWUSR},
> + "ambient", &smk_ambient_ops, 0644},
> [SMK_NET4ADDR] = {
> - "netlabel", &smk_net4addr_ops, S_IRUGO|S_IWUSR},
> + "netlabel", &smk_net4addr_ops, 0644},
> [SMK_ONLYCAP] = {
> - "onlycap", &smk_onlycap_ops, S_IRUGO|S_IWUSR},
> + "onlycap", &smk_onlycap_ops, 0644},
> [SMK_LOGGING] = {
> - "logging", &smk_logging_ops, S_IRUGO|S_IWUSR},
> + "logging", &smk_logging_ops, 0644},
> [SMK_LOAD_SELF] = {
> - "load-self", &smk_load_self_ops, S_IRUGO|S_IWUGO},
> + "load-self", &smk_load_self_ops, 0666},
> [SMK_ACCESSES] = {
> - "access", &smk_access_ops, S_IRUGO|S_IWUGO},
> + "access", &smk_access_ops, 0666},
> [SMK_MAPPED] = {
> - "mapped", &smk_mapped_ops, S_IRUGO|S_IWUSR},
> + "mapped", &smk_mapped_ops, 0644},
> [SMK_LOAD2] = {
> - "load2", &smk_load2_ops, S_IRUGO|S_IWUSR},
> + "load2", &smk_load2_ops, 0644},
> [SMK_LOAD_SELF2] = {
> - "load-self2", &smk_load_self2_ops, S_IRUGO|S_IWUGO},
> + "load-self2", &smk_load_self2_ops, 0666},
> [SMK_ACCESS2] = {
> - "access2", &smk_access2_ops, S_IRUGO|S_IWUGO},
> + "access2", &smk_access2_ops, 0666},
> [SMK_CIPSO2] = {
> - "cipso2", &smk_cipso2_ops, S_IRUGO|S_IWUSR},
> + "cipso2", &smk_cipso2_ops, 0644},
> [SMK_REVOKE_SUBJ] = {
> - "revoke-subject", &smk_revoke_subj_ops,
> - S_IRUGO|S_IWUSR},
> + "revoke-subject", &smk_revoke_subj_ops, 0644},
> [SMK_CHANGE_RULE] = {
> - "change-rule", &smk_change_rule_ops, S_IRUGO|S_IWUSR},
> + "change-rule", &smk_change_rule_ops, 0644},
> [SMK_SYSLOG] = {
> - "syslog", &smk_syslog_ops, S_IRUGO|S_IWUSR},
> + "syslog", &smk_syslog_ops, 0644},
> [SMK_PTRACE] = {
> - "ptrace", &smk_ptrace_ops, S_IRUGO|S_IWUSR},
> + "ptrace", &smk_ptrace_ops, 0644},
> #ifdef CONFIG_SECURITY_SMACK_BRINGUP
> [SMK_UNCONFINED] = {
> - "unconfined", &smk_unconfined_ops, S_IRUGO|S_IWUSR},
> + "unconfined", &smk_unconfined_ops, 0644},
> #endif
> #if IS_ENABLED(CONFIG_IPV6)
> [SMK_NET6ADDR] = {
> - "ipv6host", &smk_net6addr_ops, S_IRUGO|S_IWUSR},
> + "ipv6host", &smk_net6addr_ops, 0644},
> #endif /* CONFIG_IPV6 */
> [SMK_RELABEL_SELF] = {
> - "relabel-self", &smk_relabel_self_ops,
> - S_IRUGO|S_IWUGO},
> + "relabel-self", &smk_relabel_self_ops, 0666},
> /* last one */
> {""}
> };
> diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
> index 8d0e1b9c9c57..2069f5912469 100644
> --- a/security/tomoyo/condition.c
> +++ b/security/tomoyo/condition.c
> @@ -874,31 +874,31 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
> value = S_ISVTX;
> break;
> case TOMOYO_MODE_OWNER_READ:
> - value = S_IRUSR;
> + value = 0400;
> break;
> case TOMOYO_MODE_OWNER_WRITE:
> - value = S_IWUSR;
> + value = 0200;
> break;
> case TOMOYO_MODE_OWNER_EXECUTE:
> - value = S_IXUSR;
> + value = 0100;
> break;
> case TOMOYO_MODE_GROUP_READ:
> - value = S_IRGRP;
> + value = 0040;
> break;
> case TOMOYO_MODE_GROUP_WRITE:
> - value = S_IWGRP;
> + value = 0020;
> break;
> case TOMOYO_MODE_GROUP_EXECUTE:
> - value = S_IXGRP;
> + value = 0010;
> break;
> case TOMOYO_MODE_OTHERS_READ:
> - value = S_IROTH;
> + value = 0004;
> break;
> case TOMOYO_MODE_OTHERS_WRITE:
> - value = S_IWOTH;
> + value = 0002;
> break;
> case TOMOYO_MODE_OTHERS_EXECUTE:
> - value = S_IXOTH;
> + value = 0001;
> break;
> case TOMOYO_EXEC_ARGC:
> if (!bprm)
On Mon, 11 Jun 2018, Casey Schaufler wrote:
> If you want to break this up by security module I would take
> the Smack part as soon as James does the tree update. If James
> wants to take the whole thing at once you can add my:
>
> Acked-by: Casey Schaufler <[email protected]>
>
> for the Smack changes.
It's probably simplest for me to take them as one patch.
--
James Morris
<[email protected]>
On Tue, Jun 12, 2018 at 4:32 PM, James Morris <[email protected]> wrote:
> On Mon, 11 Jun 2018, Casey Schaufler wrote:
>
>> If you want to break this up by security module I would take
>> the Smack part as soon as James does the tree update. If James
>> wants to take the whole thing at once you can add my:
>>
>> Acked-by: Casey Schaufler <[email protected]>
>>
>> for the Smack changes.
>
> It's probably simplest for me to take them as one patch.
I would prefer if the SELinux changes were split into a separate
patch. I'm guessing John would probably want the same for the
AppArmor patches, but take his work for it, not mine.
Joe, in general I really appreciate the fixes you send, but these
patches that cross a lot of subsystem boundaries (this isn't the first
one that does this) causes unnecessary conflicts in -next and during
the merge window. Could you split your patches up from now on please?
--
paul moore
http://www.paul-moore.com
On 06/12/2018 02:12 PM, Paul Moore wrote:
> On Tue, Jun 12, 2018 at 4:32 PM, James Morris <[email protected]> wrote:
>> On Mon, 11 Jun 2018, Casey Schaufler wrote:
>>
>>> If you want to break this up by security module I would take
>>> the Smack part as soon as James does the tree update. If James
>>> wants to take the whole thing at once you can add my:
>>>
>>> Acked-by: Casey Schaufler <[email protected]>
>>>
>>> for the Smack changes.
>>
>> It's probably simplest for me to take them as one patch.
>
> I would prefer if the SELinux changes were split into a separate
> patch. I'm guessing John would probably want the same for the
> AppArmor patches, but take his work for it, not mine.
yes that would be preferred
>
> Joe, in general I really appreciate the fixes you send, but these
> patches that cross a lot of subsystem boundaries (this isn't the first
> one that does this) causes unnecessary conflicts in -next and during
> the merge window. Could you split your patches up from now on please?
>
yeah splitting patches at subsystem boundaries is highly recommended.
On Tue, 2018-06-12 at 14:29 -0700, John Johansen wrote:
> On 06/12/2018 02:12 PM, Paul Moore wrote:
> > On Tue, Jun 12, 2018 at 4:32 PM, James Morris <[email protected]> wrote:
> >> On Mon, 11 Jun 2018, Casey Schaufler wrote:
> >>
> >>> If you want to break this up by security module I would take
> >>> the Smack part as soon as James does the tree update. If James
> >>> wants to take the whole thing at once you can add my:
> >>>
> >>> Acked-by: Casey Schaufler <[email protected]>
> >>>
> >>> for the Smack changes.
> >>
> >> It's probably simplest for me to take them as one patch.
> >
> > I would prefer if the SELinux changes were split into a separate
> > patch. I'm guessing John would probably want the same for the
> > AppArmor patches, but take his work for it, not mine.
>
> yes that would be preferred
Agreed
>
> >
> > Joe, in general I really appreciate the fixes you send, but these
> > patches that cross a lot of subsystem boundaries (this isn't the first
> > one that does this) causes unnecessary conflicts in -next and during
> > the merge window. Could you split your patches up from now on please?
> >
>
> yeah splitting patches at subsystem boundaries is highly recommended.
Agreed
On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
> On Tue, Jun 12, 2018 at 4:32 PM, James Morris <[email protected]> wrote:
> > On Mon, 11 Jun 2018, Casey Schaufler wrote:
> >
> > > If you want to break this up by security module I would take
> > > the Smack part as soon as James does the tree update. If James
> > > wants to take the whole thing at once you can add my:
> > >
> > > Acked-by: Casey Schaufler <[email protected]>
> > >
> > > for the Smack changes.
> >
> > It's probably simplest for me to take them as one patch.
>
> I would prefer if the SELinux changes were split into a separate
> patch. I'm guessing John would probably want the same for the
> AppArmor patches, but take his work for it, not mine.
>
> Joe, in general I really appreciate the fixes you send, but these
> patches that cross a lot of subsystem boundaries (this isn't the first
> one that does this) causes unnecessary conflicts in -next and during
> the merge window. Could you split your patches up from now on please?
Sorry. No. Merge conflicts are inherent in this system.
There is just no good way to do this as sending a set
of per subsystem patches guarantees partial application
of the entire set.
The nominal best way is for a script to be run and
applied at the top level rather than sending a patch
at all.
If you prefer, each sub-subsystem maintainer, at
whatever granularity desired, could apply the patch
to the appropriate subsystem by using
"git am --include=<subsystem_path>".
cheers, Joe
Quoting Joe Perches ([email protected]):
> Currently security files use a mixture of octal and symbolic styles
> for permissions.
>
> Using octal and not symbolic permissions is preferred by many as more
> readable.
>
> see: https://lkml.org/lkml/2016/8/2/1945
Heh, well see also https://lkml.org/lkml/2016/8/2/2062 . Your patch
definately improves readability, but will doubtless make backpointing
future important patches more painful.
> Prefer the direct use of octal for permissions.
>
> Done using:
>
> $ git ls-files security | \
> xargs ./scripts/checkpatch.pl -f --fix-inplace --types=symbolic_perms --strict
>
> and some typing.
>
> Before: $ git grep -P -w "0[0-7]{3,3}" security | wc -l
> 53
> After: $ git grep -P -w "0[0-7]{3,3}" security | wc -l
> 136
>
> Miscellanea:
>
> o Whitespace neatening and line wrapping around these conversions.
> o Remove now superfluous parentheses around direct use of 0600
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> security/apparmor/apparmorfs.c | 5 ++--
> security/apparmor/lsm.c | 23 ++++++++---------
> security/integrity/ima/ima.h | 4 +--
> security/integrity/ima/ima_fs.c | 13 +++++-----
> security/selinux/hooks.c | 4 +--
> security/selinux/selinuxfs.c | 57 ++++++++++++++++++++---------------------
> security/smack/smack_lsm.c | 6 ++---
> security/smack/smackfs.c | 46 ++++++++++++++++-----------------
> security/tomoyo/condition.c | 18 ++++++-------
> 9 files changed, 85 insertions(+), 91 deletions(-)
>
> diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> index 949dd8a48164..c09dc0f3c3fe 100644
> --- a/security/apparmor/apparmorfs.c
> +++ b/security/apparmor/apparmorfs.c
> @@ -2426,10 +2426,9 @@ static int aa_mk_null_file(struct dentry *parent)
> }
>
> inode->i_ino = get_next_ino();
> - inode->i_mode = S_IFCHR | S_IRUGO | S_IWUGO;
> + inode->i_mode = S_IFCHR | 0666;
> inode->i_atime = inode->i_mtime = inode->i_ctime = current_time(inode);
> - init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO,
> - MKDEV(MEM_MAJOR, 3));
> + init_special_inode(inode, S_IFCHR | 0666, MKDEV(MEM_MAJOR, 3));
> d_instantiate(dentry, inode);
> aa_null.dentry = dget(dentry);
> aa_null.mnt = mntget(mount);
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index fbb08bc78bee..6759a70918de 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1255,45 +1255,42 @@ static int param_get_mode(char *buffer, const struct kernel_param *kp);
> /* AppArmor global enforcement switch - complain, enforce, kill */
> enum profile_mode aa_g_profile_mode = APPARMOR_ENFORCE;
> module_param_call(mode, param_set_mode, param_get_mode,
> - &aa_g_profile_mode, S_IRUSR | S_IWUSR);
> + &aa_g_profile_mode, 0600);
>
> /* whether policy verification hashing is enabled */
> bool aa_g_hash_policy = IS_ENABLED(CONFIG_SECURITY_APPARMOR_HASH_DEFAULT);
> #ifdef CONFIG_SECURITY_APPARMOR_HASH
> -module_param_named(hash_policy, aa_g_hash_policy, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(hash_policy, aa_g_hash_policy, aabool, 0600);
> #endif
>
> /* Debug mode */
> bool aa_g_debug = IS_ENABLED(CONFIG_SECURITY_APPARMOR_DEBUG_MESSAGES);
> -module_param_named(debug, aa_g_debug, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(debug, aa_g_debug, aabool, 0600);
>
> /* Audit mode */
> enum audit_mode aa_g_audit;
> -module_param_call(audit, param_set_audit, param_get_audit,
> - &aa_g_audit, S_IRUSR | S_IWUSR);
> +module_param_call(audit, param_set_audit, param_get_audit, &aa_g_audit, 0600);
>
> /* Determines if audit header is included in audited messages. This
> * provides more context if the audit daemon is not running
> */
> bool aa_g_audit_header = true;
> -module_param_named(audit_header, aa_g_audit_header, aabool,
> - S_IRUSR | S_IWUSR);
> +module_param_named(audit_header, aa_g_audit_header, aabool, 0600);
>
> /* lock out loading/removal of policy
> * TODO: add in at boot loading of policy, which is the only way to
> * load policy, if lock_policy is set
> */
> bool aa_g_lock_policy;
> -module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy,
> - S_IRUSR | S_IWUSR);
> +module_param_named(lock_policy, aa_g_lock_policy, aalockpolicy, 0600);
>
> /* Syscall logging mode */
> bool aa_g_logsyscall;
> -module_param_named(logsyscall, aa_g_logsyscall, aabool, S_IRUSR | S_IWUSR);
> +module_param_named(logsyscall, aa_g_logsyscall, aabool, 0600);
>
> /* Maximum pathname length before accesses will start getting rejected */
> unsigned int aa_g_path_max = 2 * PATH_MAX;
> -module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
> +module_param_named(path_max, aa_g_path_max, aauint, 0400);
>
> /* Determines how paranoid loading of policy is and how much verification
> * on the loaded policy is done.
> @@ -1301,11 +1298,11 @@ module_param_named(path_max, aa_g_path_max, aauint, S_IRUSR);
> * that none root users (user namespaces) can load policy.
> */
> bool aa_g_paranoid_load = true;
> -module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
> +module_param_named(paranoid_load, aa_g_paranoid_load, aabool, 0444);
>
> /* Boot time disable flag */
> static bool apparmor_enabled = CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE;
> -module_param_named(enabled, apparmor_enabled, bool, S_IRUGO);
> +module_param_named(enabled, apparmor_enabled, bool, 0444);
>
> static int __init apparmor_enabled_setup(char *str)
> {
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 354bb5716ce3..3f7707b8aaa7 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -314,9 +314,9 @@ static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> #endif /* CONFIG_IMA_LSM_RULES */
>
> #ifdef CONFIG_IMA_READ_POLICY
> -#define POLICY_FILE_FLAGS (S_IWUSR | S_IRUSR)
> +#define POLICY_FILE_FLAGS 0600
> #else
> -#define POLICY_FILE_FLAGS S_IWUSR
> +#define POLICY_FILE_FLAGS 0200
> #endif /* CONFIG_IMA_READ_POLICY */
>
> #endif /* __LINUX_IMA_H */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index ae9d5c766a3c..81700df83f51 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -439,7 +439,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
> #elif defined(CONFIG_IMA_WRITE_POLICY)
> clear_bit(IMA_FS_BUSY, &ima_fs_flags);
> #elif defined(CONFIG_IMA_READ_POLICY)
> - inode->i_mode &= ~S_IWUSR;
> + inode->i_mode &= ~0200;
> #endif
> return 0;
> }
> @@ -465,28 +465,29 @@ int __init ima_fs_init(void)
>
> binary_runtime_measurements =
> securityfs_create_file("binary_runtime_measurements",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> + 0440, ima_dir, NULL,
> &ima_measurements_ops);
> if (IS_ERR(binary_runtime_measurements))
> goto out;
>
> ascii_runtime_measurements =
> securityfs_create_file("ascii_runtime_measurements",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> + 0440, ima_dir, NULL,
> &ima_ascii_measurements_ops);
> if (IS_ERR(ascii_runtime_measurements))
> goto out;
>
> runtime_measurements_count =
> securityfs_create_file("runtime_measurements_count",
> - S_IRUSR | S_IRGRP, ima_dir, NULL,
> + 0440, ima_dir, NULL,
> &ima_measurements_count_ops);
> if (IS_ERR(runtime_measurements_count))
> goto out;
>
> violations =
> - securityfs_create_file("violations", S_IRUSR | S_IRGRP,
> - ima_dir, NULL, &ima_htable_violations_ops);
> + securityfs_create_file("violations",
> + 0440, ima_dir, NULL,
> + &ima_htable_violations_ops);
> if (IS_ERR(violations))
> goto out;
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index a85fac3345df..8ae043be8782 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6336,9 +6336,9 @@ static int selinux_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
> u32 av = 0;
>
> av = 0;
> - if (flag & S_IRUGO)
> + if (flag & 0444)
> av |= IPC__UNIX_READ;
> - if (flag & S_IWUGO)
> + if (flag & 0222)
> av |= IPC__UNIX_WRITE;
>
> if (av == 0)
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index f3d374d2ca04..bfecac19ba92 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1376,7 +1376,7 @@ static int sel_make_bools(struct selinux_fs_info *fsi)
> goto out;
>
> ret = -ENOMEM;
> - inode = sel_make_inode(dir->d_sb, S_IFREG | S_IRUGO | S_IWUSR);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0644);
> if (!inode)
> goto out;
>
> @@ -1582,10 +1582,10 @@ static int sel_make_avc_files(struct dentry *dir)
> int i;
> static const struct tree_descr files[] = {
> { "cache_threshold",
> - &sel_avc_cache_threshold_ops, S_IRUGO|S_IWUSR },
> - { "hash_stats", &sel_avc_hash_stats_ops, S_IRUGO },
> + &sel_avc_cache_threshold_ops, 0644 },
> + { "hash_stats", &sel_avc_hash_stats_ops, 0444 },
> #ifdef CONFIG_SECURITY_SELINUX_AVC_STATS
> - { "cache_stats", &sel_avc_cache_stats_ops, S_IRUGO },
> + { "cache_stats", &sel_avc_cache_stats_ops, 0444 },
> #endif
> };
>
> @@ -1643,7 +1643,7 @@ static int sel_make_initcon_files(struct dentry *dir)
> if (!dentry)
> return -ENOMEM;
>
> - inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
> if (!inode)
> return -ENOMEM;
>
> @@ -1744,7 +1744,7 @@ static int sel_make_perm_files(char *objclass, int classvalue,
> goto out;
>
> rc = -ENOMEM;
> - inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
> if (!inode)
> goto out;
>
> @@ -1774,7 +1774,7 @@ static int sel_make_class_dir_entries(char *classname, int index,
> if (!dentry)
> return -ENOMEM;
>
> - inode = sel_make_inode(dir->d_sb, S_IFREG|S_IRUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFREG | 0444);
> if (!inode)
> return -ENOMEM;
>
> @@ -1870,7 +1870,7 @@ static struct dentry *sel_make_dir(struct dentry *dir, const char *name,
> if (!dentry)
> return ERR_PTR(-ENOMEM);
>
> - inode = sel_make_inode(dir->d_sb, S_IFDIR | S_IRUGO | S_IXUGO);
> + inode = sel_make_inode(dir->d_sb, S_IFDIR | 0555);
> if (!inode) {
> dput(dentry);
> return ERR_PTR(-ENOMEM);
> @@ -1899,25 +1899,24 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
> struct inode_security_struct *isec;
>
> static const struct tree_descr selinux_files[] = {
> - [SEL_LOAD] = {"load", &sel_load_ops, S_IRUSR|S_IWUSR},
> - [SEL_ENFORCE] = {"enforce", &sel_enforce_ops, S_IRUGO|S_IWUSR},
> - [SEL_CONTEXT] = {"context", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_ACCESS] = {"access", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_CREATE] = {"create", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_RELABEL] = {"relabel", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_USER] = {"user", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_POLICYVERS] = {"policyvers", &sel_policyvers_ops, S_IRUGO},
> - [SEL_COMMIT_BOOLS] = {"commit_pending_bools", &sel_commit_bools_ops, S_IWUSR},
> - [SEL_MLS] = {"mls", &sel_mls_ops, S_IRUGO},
> - [SEL_DISABLE] = {"disable", &sel_disable_ops, S_IWUSR},
> - [SEL_MEMBER] = {"member", &transaction_ops, S_IRUGO|S_IWUGO},
> - [SEL_CHECKREQPROT] = {"checkreqprot", &sel_checkreqprot_ops, S_IRUGO|S_IWUSR},
> - [SEL_REJECT_UNKNOWN] = {"reject_unknown", &sel_handle_unknown_ops, S_IRUGO},
> - [SEL_DENY_UNKNOWN] = {"deny_unknown", &sel_handle_unknown_ops, S_IRUGO},
> - [SEL_STATUS] = {"status", &sel_handle_status_ops, S_IRUGO},
> - [SEL_POLICY] = {"policy", &sel_policy_ops, S_IRUGO},
> - [SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops,
> - S_IWUGO},
> + [SEL_LOAD] = {"load", &sel_load_ops, 0600},
> + [SEL_ENFORCE] = {"enforce", &sel_enforce_ops, 0644},
> + [SEL_CONTEXT] = {"context", &transaction_ops, 0666},
> + [SEL_ACCESS] = {"access", &transaction_ops, 0666},
> + [SEL_CREATE] = {"create", &transaction_ops, 0666},
> + [SEL_RELABEL] = {"relabel", &transaction_ops, 0666},
> + [SEL_USER] = {"user", &transaction_ops, 0666},
> + [SEL_POLICYVERS] = {"policyvers", &sel_policyvers_ops, 0444},
> + [SEL_COMMIT_BOOLS] = {"commit_pending_bools", &sel_commit_bools_ops, 0200},
> + [SEL_MLS] = {"mls", &sel_mls_ops, 0444},
> + [SEL_DISABLE] = {"disable", &sel_disable_ops, 0200},
> + [SEL_MEMBER] = {"member", &transaction_ops, 0666},
> + [SEL_CHECKREQPROT] = {"checkreqprot", &sel_checkreqprot_ops, 0644},
> + [SEL_REJECT_UNKNOWN] = {"reject_unknown", &sel_handle_unknown_ops, 0444},
> + [SEL_DENY_UNKNOWN] = {"deny_unknown", &sel_handle_unknown_ops, 0444},
> + [SEL_STATUS] = {"status", &sel_handle_status_ops, 0444},
> + [SEL_POLICY] = {"policy", &sel_policy_ops, 0444},
> + [SEL_VALIDATE_TRANS] = {"validatetrans", &sel_transition_ops, 0222},
> /* last one */ {""}
> };
>
> @@ -1943,7 +1942,7 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
> goto err;
>
> ret = -ENOMEM;
> - inode = sel_make_inode(sb, S_IFCHR | S_IRUGO | S_IWUGO);
> + inode = sel_make_inode(sb, S_IFCHR | 0666);
> if (!inode)
> goto err;
>
> @@ -1953,7 +1952,7 @@ static int sel_fill_super(struct super_block *sb, void *data, int silent)
> isec->sclass = SECCLASS_CHR_FILE;
> isec->initialized = LABEL_INITIALIZED;
>
> - init_special_inode(inode, S_IFCHR | S_IRUGO | S_IWUGO, MKDEV(MEM_MAJOR, 3));
> + init_special_inode(inode, S_IFCHR | 0666, MKDEV(MEM_MAJOR, 3));
> d_add(dentry, inode);
>
> dentry = sel_make_dir(sb->s_root, "avc", &fsi->last_ino);
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index dcb976f98df2..8953440c6559 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2945,11 +2945,11 @@ static int smack_flags_to_may(int flags)
> {
> int may = 0;
>
> - if (flags & S_IRUGO)
> + if (flags & 0444)
> may |= MAY_READ;
> - if (flags & S_IWUGO)
> + if (flags & 0222)
> may |= MAY_WRITE;
> - if (flags & S_IXUGO)
> + if (flags & 0111)
> may |= MAY_EXEC;
>
> return may;
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index f6482e53d55a..270cd3a308f0 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -2857,55 +2857,53 @@ static int smk_fill_super(struct super_block *sb, void *data, int silent)
>
> static const struct tree_descr smack_files[] = {
> [SMK_LOAD] = {
> - "load", &smk_load_ops, S_IRUGO|S_IWUSR},
> + "load", &smk_load_ops, 0644},
> [SMK_CIPSO] = {
> - "cipso", &smk_cipso_ops, S_IRUGO|S_IWUSR},
> + "cipso", &smk_cipso_ops, 0644},
> [SMK_DOI] = {
> - "doi", &smk_doi_ops, S_IRUGO|S_IWUSR},
> + "doi", &smk_doi_ops, 0644},
> [SMK_DIRECT] = {
> - "direct", &smk_direct_ops, S_IRUGO|S_IWUSR},
> + "direct", &smk_direct_ops, 0644},
> [SMK_AMBIENT] = {
> - "ambient", &smk_ambient_ops, S_IRUGO|S_IWUSR},
> + "ambient", &smk_ambient_ops, 0644},
> [SMK_NET4ADDR] = {
> - "netlabel", &smk_net4addr_ops, S_IRUGO|S_IWUSR},
> + "netlabel", &smk_net4addr_ops, 0644},
> [SMK_ONLYCAP] = {
> - "onlycap", &smk_onlycap_ops, S_IRUGO|S_IWUSR},
> + "onlycap", &smk_onlycap_ops, 0644},
> [SMK_LOGGING] = {
> - "logging", &smk_logging_ops, S_IRUGO|S_IWUSR},
> + "logging", &smk_logging_ops, 0644},
> [SMK_LOAD_SELF] = {
> - "load-self", &smk_load_self_ops, S_IRUGO|S_IWUGO},
> + "load-self", &smk_load_self_ops, 0666},
> [SMK_ACCESSES] = {
> - "access", &smk_access_ops, S_IRUGO|S_IWUGO},
> + "access", &smk_access_ops, 0666},
> [SMK_MAPPED] = {
> - "mapped", &smk_mapped_ops, S_IRUGO|S_IWUSR},
> + "mapped", &smk_mapped_ops, 0644},
> [SMK_LOAD2] = {
> - "load2", &smk_load2_ops, S_IRUGO|S_IWUSR},
> + "load2", &smk_load2_ops, 0644},
> [SMK_LOAD_SELF2] = {
> - "load-self2", &smk_load_self2_ops, S_IRUGO|S_IWUGO},
> + "load-self2", &smk_load_self2_ops, 0666},
> [SMK_ACCESS2] = {
> - "access2", &smk_access2_ops, S_IRUGO|S_IWUGO},
> + "access2", &smk_access2_ops, 0666},
> [SMK_CIPSO2] = {
> - "cipso2", &smk_cipso2_ops, S_IRUGO|S_IWUSR},
> + "cipso2", &smk_cipso2_ops, 0644},
> [SMK_REVOKE_SUBJ] = {
> - "revoke-subject", &smk_revoke_subj_ops,
> - S_IRUGO|S_IWUSR},
> + "revoke-subject", &smk_revoke_subj_ops, 0644},
> [SMK_CHANGE_RULE] = {
> - "change-rule", &smk_change_rule_ops, S_IRUGO|S_IWUSR},
> + "change-rule", &smk_change_rule_ops, 0644},
> [SMK_SYSLOG] = {
> - "syslog", &smk_syslog_ops, S_IRUGO|S_IWUSR},
> + "syslog", &smk_syslog_ops, 0644},
> [SMK_PTRACE] = {
> - "ptrace", &smk_ptrace_ops, S_IRUGO|S_IWUSR},
> + "ptrace", &smk_ptrace_ops, 0644},
> #ifdef CONFIG_SECURITY_SMACK_BRINGUP
> [SMK_UNCONFINED] = {
> - "unconfined", &smk_unconfined_ops, S_IRUGO|S_IWUSR},
> + "unconfined", &smk_unconfined_ops, 0644},
> #endif
> #if IS_ENABLED(CONFIG_IPV6)
> [SMK_NET6ADDR] = {
> - "ipv6host", &smk_net6addr_ops, S_IRUGO|S_IWUSR},
> + "ipv6host", &smk_net6addr_ops, 0644},
> #endif /* CONFIG_IPV6 */
> [SMK_RELABEL_SELF] = {
> - "relabel-self", &smk_relabel_self_ops,
> - S_IRUGO|S_IWUGO},
> + "relabel-self", &smk_relabel_self_ops, 0666},
> /* last one */
> {""}
> };
> diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
> index 8d0e1b9c9c57..2069f5912469 100644
> --- a/security/tomoyo/condition.c
> +++ b/security/tomoyo/condition.c
> @@ -874,31 +874,31 @@ bool tomoyo_condition(struct tomoyo_request_info *r,
> value = S_ISVTX;
> break;
> case TOMOYO_MODE_OWNER_READ:
> - value = S_IRUSR;
> + value = 0400;
> break;
> case TOMOYO_MODE_OWNER_WRITE:
> - value = S_IWUSR;
> + value = 0200;
> break;
> case TOMOYO_MODE_OWNER_EXECUTE:
> - value = S_IXUSR;
> + value = 0100;
> break;
> case TOMOYO_MODE_GROUP_READ:
> - value = S_IRGRP;
> + value = 0040;
> break;
> case TOMOYO_MODE_GROUP_WRITE:
> - value = S_IWGRP;
> + value = 0020;
> break;
> case TOMOYO_MODE_GROUP_EXECUTE:
> - value = S_IXGRP;
> + value = 0010;
> break;
> case TOMOYO_MODE_OTHERS_READ:
> - value = S_IROTH;
> + value = 0004;
> break;
> case TOMOYO_MODE_OTHERS_WRITE:
> - value = S_IWOTH;
> + value = 0002;
> break;
> case TOMOYO_MODE_OTHERS_EXECUTE:
> - value = S_IXOTH;
> + value = 0001;
> break;
> case TOMOYO_EXEC_ARGC:
> if (!bprm)
> --
> 2.15.0
On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches <[email protected]> wrote:
> On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
>> Joe, in general I really appreciate the fixes you send, but these
>> patches that cross a lot of subsystem boundaries (this isn't the first
>> one that does this) causes unnecessary conflicts in -next and during
>> the merge window. Could you split your patches up from now on please?
>
> Sorry. No. Merge conflicts are inherent in this system.
Yes, merge conflicts are inherent in this system when one makes a
single change which impacts multiple subsystems, e.g. changing a core
kernel function which is called by multiple subsystems. However, that
isn't what this patch does, it makes a number of self-contained
changes across multiple subsystems; there are no cross-subsystem
dependencies in this patch. You are increasing the likelihood of
conflicts for no good reason; that is why I'm asking you to split this
patch and others like it.
> There is just no good way to do this as sending a set
> of per subsystem patches guarantees partial application
> of the entire set.
Please explain why all of these changes need to be made at the same
time? Looking quickly at the patch it would appear that each chunk of
the patch could be applied independently and the kernel would still
build and operate correctly. I'm not suggesting that you decompose
this patch with that level of granularity, that would be silly, but
splitting this patch (and many of the others you tend to submit) at
subsystem boundaries should be possible.
Further, as one could see from the responses of the other LSM
maintainers, splitting this patch is not just possible, it is
desirable.
> If you prefer, each sub-subsystem maintainer, at
> whatever granularity desired, could apply the patch
> to the appropriate subsystem by using
> "git am --include=<subsystem_path>".
Or you could split the patch up by subsystem before posting, like so
many other developers do already.
--
paul moore
http://www.paul-moore.com
On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote:
> On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches <[email protected]> wrote:
> > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
> > > Joe, in general I really appreciate the fixes you send, but these
> > > patches that cross a lot of subsystem boundaries (this isn't the first
> > > one that does this) causes unnecessary conflicts in -next and during
> > > the merge window. Could you split your patches up from now on please?
> >
> > Sorry. No. Merge conflicts are inherent in this system.
>
> Yes, merge conflicts are inherent in this system when one makes a
> single change which impacts multiple subsystems, e.g. changing a core
> kernel function which is called by multiple subsystems. However, that
> isn't what this patch does, it makes a number of self-contained
> changes across multiple subsystems; there are no cross-subsystem
> dependencies in this patch. You are increasing the likelihood of
> conflicts for no good reason; that is why I'm asking you to split this
> patch and others like it.
No. History shows with high certainty that splitting
patches like this across multiple subsystems of a primary
subsystem means that the entire patchset is not completely
applied.
It's _much_ simpler and provides a generic mechanism to
get the entire patch applied to send a single patch to the
top level subsystem maintainer.
On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches <[email protected]> wrote:
> On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote:
>> On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches <[email protected]> wrote:
>> > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
>> > > Joe, in general I really appreciate the fixes you send, but these
>> > > patches that cross a lot of subsystem boundaries (this isn't the first
>> > > one that does this) causes unnecessary conflicts in -next and during
>> > > the merge window. Could you split your patches up from now on please?
>> >
>> > Sorry. No. Merge conflicts are inherent in this system.
>>
>> Yes, merge conflicts are inherent in this system when one makes a
>> single change which impacts multiple subsystems, e.g. changing a core
>> kernel function which is called by multiple subsystems. However, that
>> isn't what this patch does, it makes a number of self-contained
>> changes across multiple subsystems; there are no cross-subsystem
>> dependencies in this patch. You are increasing the likelihood of
>> conflicts for no good reason; that is why I'm asking you to split this
>> patch and others like it.
>
> No. History shows with high certainty that splitting
> patches like this across multiple subsystems of a primary
> subsystem means that the entire patchset is not completely
> applied.
I think that is due more to a lack of effort on the part of the patch
author to keep pushing the individual patches.
> It's _much_ simpler and provides a generic mechanism to
> get the entire patch applied to send a single patch to the
> top level subsystem maintainer.
I understand it is simpler for you, but it is more difficult for everyone else.
Further, where the LSMs are concerned, there is no "top level
subsystem maintainer" anymore. SELinux and AppArmor send pull
requests directly to Linus.
--
paul moore
http://www.paul-moore.com
On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote:
> On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches <[email protected]> wrote:
> > On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote:
> > > On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches <[email protected]> wrote:
> > > > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
> > > > > Joe, in general I really appreciate the fixes you send, but these
> > > > > patches that cross a lot of subsystem boundaries (this isn't the first
> > > > > one that does this) causes unnecessary conflicts in -next and during
> > > > > the merge window. Could you split your patches up from now on please?
> > > >
> > > > Sorry. No. Merge conflicts are inherent in this system.
> > >
> > > Yes, merge conflicts are inherent in this system when one makes a
> > > single change which impacts multiple subsystems, e.g. changing a core
> > > kernel function which is called by multiple subsystems. However, that
> > > isn't what this patch does, it makes a number of self-contained
> > > changes across multiple subsystems; there are no cross-subsystem
> > > dependencies in this patch. You are increasing the likelihood of
> > > conflicts for no good reason; that is why I'm asking you to split this
> > > patch and others like it.
> >
> > No. History shows with high certainty that splitting
> > patches like this across multiple subsystems of a primary
> > subsystem means that the entire patchset is not completely
> > applied.
>
> I think that is due more to a lack of effort on the part of the patch
> author to keep pushing the individual patches.
Nope. Try again.
Resistance to change and desire for status quo
occurs in many subsystems.
> > It's _much_ simpler and provides a generic mechanism to
> > get the entire patch applied to send a single patch to the
> > top level subsystem maintainer.
>
> I understand it is simpler for you, but it is more difficult for everyone else.
Not true.
It's simply a matter of merge resolution being pushed down
where and when necessary.
See changes like the additions of the SPDX license tags.
> Further, where the LSMs are concerned, there is no "top level
> subsystem maintainer" anymore. SELinux and AppArmor send pull
> requests directly to Linus.
MAINTAINERS-SECURITY SUBSYSTEM
MAINTAINERS-M: James Morris <[email protected]>
MAINTAINERS-M: "Serge E. Hallyn" <[email protected]>
MAINTAINERS-L: [email protected] (suggested Cc:)
MAINTAINERS-T: git git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
MAINTAINERS-W: http://kernsec.org/
MAINTAINERS-S: Supported
MAINTAINERS:F: security/
MAINTAINERS-
If James is not approving or merging security/selinux or
security/tomoyo then perhaps the F: entries could be
augmented with appropriate X: entries or made specific
by using specific entries like:
F: security/*
F: security/integrity/
F: security/keys/
On Wed, Jun 13, 2018 at 3:30 PM, Joe Perches <[email protected]> wrote:
> On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote:
>> On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches <[email protected]> wrote:
>> > On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote:
>> > > On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches <[email protected]> wrote:
>> > > > On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
>> > > > > Joe, in general I really appreciate the fixes you send, but these
>> > > > > patches that cross a lot of subsystem boundaries (this isn't the first
>> > > > > one that does this) causes unnecessary conflicts in -next and during
>> > > > > the merge window. Could you split your patches up from now on please?
>> > > >
>> > > > Sorry. No. Merge conflicts are inherent in this system.
>> > >
>> > > Yes, merge conflicts are inherent in this system when one makes a
>> > > single change which impacts multiple subsystems, e.g. changing a core
>> > > kernel function which is called by multiple subsystems. However, that
>> > > isn't what this patch does, it makes a number of self-contained
>> > > changes across multiple subsystems; there are no cross-subsystem
>> > > dependencies in this patch. You are increasing the likelihood of
>> > > conflicts for no good reason; that is why I'm asking you to split this
>> > > patch and others like it.
>> >
>> > No. History shows with high certainty that splitting
>> > patches like this across multiple subsystems of a primary
>> > subsystem means that the entire patchset is not completely
>> > applied.
>>
>> I think that is due more to a lack of effort on the part of the patch
>> author to keep pushing the individual patches.
>
> Nope. Try again.
>
> Resistance to change and desire for status quo
> occurs in many subsystems.
Which gets back to the need for persistence on the part of the patch
author. If your solution to a stubborn susbsystem is to go around
them by convincing another, potentially unrelated subsystem, to merge
the patch then I firmly believe you are doing it wrong.
>> > It's _much_ simpler and provides a generic mechanism to
>> > get the entire patch applied to send a single patch to the
>> > top level subsystem maintainer.
>>
>> I understand it is simpler for you, but it is more difficult for everyone else.
>
> Not true.
I think we are at the agree to disagree stage.
The way you have structured this patch it makes it easier for you to
submit, but makes it potentially more difficult for me (likely other
LSM maintainers too), the -next folks, and Linus.
> It's simply a matter of merge resolution being pushed down
> where and when necessary.
>
> See changes like the additions of the SPDX license tags.
Please don't even try to suggest that this trivial patch you are
proposing is even remotely as significant as the SPDX change. There
are always going to be exceptions to every rule, and with each
exception there needs to be a solid reason behind the change. The
SPDX change had a legitimate reason (legal concern) for doing it the
way it was done; this patch isn't close to that level of concern.
>> Further, where the LSMs are concerned, there is no "top level
>> subsystem maintainer" anymore. SELinux and AppArmor send pull
>> requests directly to Linus.
>
> MAINTAINERS-SECURITY SUBSYSTEM
> MAINTAINERS-M: James Morris <[email protected]>
> MAINTAINERS-M: "Serge E. Hallyn" <[email protected]>
> MAINTAINERS-L: [email protected] (suggested Cc:)
> MAINTAINERS-T: git git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
> MAINTAINERS-W: http://kernsec.org/
> MAINTAINERS-S: Supported
> MAINTAINERS:F: security/
> MAINTAINERS-
>
> If James is not approving or merging security/selinux or
> security/tomoyo then perhaps the F: entries could be
> augmented with appropriate X: entries or made specific
> by using specific entries like:
>
> F: security/*
> F: security/integrity/
> F: security/keys/
That is a good point. I'll put together a patch for selinux/next as
soon as the merge window closes. I'll let the other LSMs do as they
see fit. As I said previously, I believe the only other LSM that
sends directly to Linux is AppArmor.
--
paul moore
http://www.paul-moore.com
On 6/13/2018 12:57 PM, Paul Moore wrote:
> On Wed, Jun 13, 2018 at 3:30 PM, Joe Perches <[email protected]> wrote:
>> On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote:
>>> On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches <[email protected]> wrote:
>>>> On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote:
>>>>> On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches <[email protected]> wrote:
>>>>>> On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
>>>>>>> Joe, in general I really appreciate the fixes you send, but these
>>>>>>> patches that cross a lot of subsystem boundaries (this isn't the first
>>>>>>> one that does this) causes unnecessary conflicts in -next and during
>>>>>>> the merge window. Could you split your patches up from now on please?
>>>>>> Sorry. No. Merge conflicts are inherent in this system.
>>>>> Yes, merge conflicts are inherent in this system when one makes a
>>>>> single change which impacts multiple subsystems, e.g. changing a core
>>>>> kernel function which is called by multiple subsystems. However, that
>>>>> isn't what this patch does, it makes a number of self-contained
>>>>> changes across multiple subsystems; there are no cross-subsystem
>>>>> dependencies in this patch. You are increasing the likelihood of
>>>>> conflicts for no good reason; that is why I'm asking you to split this
>>>>> patch and others like it.
>>>> No. History shows with high certainty that splitting
>>>> patches like this across multiple subsystems of a primary
>>>> subsystem means that the entire patchset is not completely
>>>> applied.
>>> I think that is due more to a lack of effort on the part of the patch
>>> author to keep pushing the individual patches.
>> Nope. Try again.
>>
>> Resistance to change and desire for status quo
>> occurs in many subsystems.
> Which gets back to the need for persistence on the part of the patch
> author. If your solution to a stubborn susbsystem is to go around
> them by convincing another, potentially unrelated subsystem, to merge
> the patch then I firmly believe you are doing it wrong.
>
>>>> It's _much_ simpler and provides a generic mechanism to
>>>> get the entire patch applied to send a single patch to the
>>>> top level subsystem maintainer.
>>> I understand it is simpler for you, but it is more difficult for everyone else.
>> Not true.
> I think we are at the agree to disagree stage.
>
> The way you have structured this patch it makes it easier for you to
> submit, but makes it potentially more difficult for me (likely other
> LSM maintainers too), the -next folks, and Linus.
I am agreeing with Paul. There is no reason that I/he should
be compelled to accept the Smack/SELinux patches because he/I
accepted the SELinux/Smack bits.
>
>> It's simply a matter of merge resolution being pushed down
>> where and when necessary.
>>
>> See changes like the additions of the SPDX license tags.
> Please don't even try to suggest that this trivial patch you are
> proposing is even remotely as significant as the SPDX change. There
> are always going to be exceptions to every rule, and with each
> exception there needs to be a solid reason behind the change. The
> SPDX change had a legitimate reason (legal concern) for doing it the
> way it was done; this patch isn't close to that level of concern.
>
>>> Further, where the LSMs are concerned, there is no "top level
>>> subsystem maintainer" anymore. SELinux and AppArmor send pull
>>> requests directly to Linus.
>> MAINTAINERS-SECURITY SUBSYSTEM
>> MAINTAINERS-M: James Morris <[email protected]>
>> MAINTAINERS-M: "Serge E. Hallyn" <[email protected]>
>> MAINTAINERS-L: [email protected] (suggested Cc:)
>> MAINTAINERS-T: git git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git
>> MAINTAINERS-W: http://kernsec.org/
>> MAINTAINERS-S: Supported
>> MAINTAINERS:F: security/
>> MAINTAINERS-
>>
>> If James is not approving or merging security/selinux or
>> security/tomoyo then perhaps the F: entries could be
>> augmented with appropriate X: entries or made specific
>> by using specific entries like:
>>
>> F: security/*
>> F: security/integrity/
>> F: security/keys/
There are already F: entries for security/selinux, security/smack
and security/apparmor so I don't get your point.
> That is a good point. I'll put together a patch for selinux/next as
> soon as the merge window closes. I'll let the other LSMs do as they
> see fit. As I said previously, I believe the only other LSM that
> sends directly to Linux is AppArmor.
Smack will continue using the security subsystem so long as
James offers the service. There's overhead in setting up the
environment for sending directly to Linus that I'm in no rush
to incur.
On Wed, Jun 13, 2018 at 5:14 PM, Casey Schaufler <[email protected]> wrote:
> On 6/13/2018 12:57 PM, Paul Moore wrote:
>> On Wed, Jun 13, 2018 at 3:30 PM, Joe Perches <[email protected]> wrote:
>>> On Wed, 2018-06-13 at 12:19 -0400, Paul Moore wrote:
>>>> On Wed, Jun 13, 2018 at 12:04 PM, Joe Perches <[email protected]> wrote:
>>>>> On Wed, 2018-06-13 at 11:49 -0400, Paul Moore wrote:
>>>>>> On Tue, Jun 12, 2018 at 8:29 PM, Joe Perches <[email protected]> wrote:
>>>>>>> On Tue, 2018-06-12 at 17:12 -0400, Paul Moore wrote:
...
>>> If James is not approving or merging security/selinux or
>>> security/tomoyo then perhaps the F: entries could be
>>> augmented with appropriate X: entries or made specific
>>> by using specific entries like:
>>>
>>> F: security/*
>>> F: security/integrity/
>>> F: security/keys/
>
> There are already F: entries for security/selinux, security/smack
> and security/apparmor so I don't get your point.
Perhaps I've interpreted this the wrong way, but I took this to mean
that those security subsystems which don't flow through James should
use the X: entry to exclude themselves. For example, here is a quick
diff to exclude SELinux:
diff --git a/MAINTAINERS b/MAINTAINERS
index c13b9fb3be0b..dc0b31121459 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12771,6 +12771,7 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/g>
W: http://kernsec.org/
S: Supported
F: security/
+X: security/selinux/
SELINUX SECURITY MODULE
M: Paul Moore <[email protected]>
--
paul moore
http://www.paul-moore.com
On Wed, 2018-06-13 at 10:19 -0500, Serge E. Hallyn wrote:
> Using octal and not symbolic permissions is preferred by many as more
> > readable.
> >
> > see: https://lkml.org/lkml/2016/8/2/1945
>
> Heh, well see also https://lkml.org/lkml/2016/8/2/2062 .
We all love Al.
I did reply then too.
cheers, Joe