| Message ID | 20220104170416.1923685-8-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
Hi Stefan, Nobody refers to the IMA securityfs files as dentries. The Subject line is suppose to provide a hint about the patch. How about changing the "Subject" line to "ima: Move IMA securityfs files into ima_namespaces or onto stack". On Tue, 2022-01-04 at 12:04 -0500, Stefan Berger wrote: > From: Stefan Berger <stefanb@linux.ibm.com> > > Move the policy file dentry into the ima_namespace for reuse by > virtualized SecurityFS and for being able to remove it from > the filesystem. Move the other dentries onto the stack. Missing is an explanation why the other IMA securityfs files can be on the stack. Maybe start out by saying that the ns_ima_init securityfs files are never deleted. Then transition into the IMA namespaced securityfs files and how they will be deleted. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > security/integrity/ima/ima.h | 2 ++ > security/integrity/ima/ima_fs.c | 32 ++++++++++++++++++-------------- > 2 files changed, 20 insertions(+), 14 deletions(-) > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > index 82b3f6a98320..224b09617c52 100644 > --- a/security/integrity/ima/ima.h > +++ b/security/integrity/ima/ima.h > @@ -140,6 +140,8 @@ struct ima_namespace { > struct mutex ima_write_mutex; > unsigned long ima_fs_flags; > int valid_policy; > + > + struct dentry *policy_dentry; None of the other securityfs files are renamed. Why is "ima_policy" being renamed to "policy_dentry"? If there is a need, it should be documented in the patch description. thanks, Mimi > } __randomize_layout; > extern struct ima_namespace init_ima_ns; >
On 1/13/22 15:28, Mimi Zohar wrote: > Hi Stefan, > > Nobody refers to the IMA securityfs files as dentries. The Subject > line is suppose to provide a hint about the patch. How about changing > the "Subject" line to "ima: Move IMA securityfs files into > ima_namespaces or onto stack". > > On Tue, 2022-01-04 at 12:04 -0500, Stefan Berger wrote: >> From: Stefan Berger <stefanb@linux.ibm.com> >> >> Move the policy file dentry into the ima_namespace for reuse by >> virtualized SecurityFS and for being able to remove it from >> the filesystem. Move the other dentries onto the stack. > Missing is an explanation why the other IMA securityfs files can be on > the stack. Maybe start out by saying that the ns_ima_init securityfs > files are never deleted. Then transition into the IMA namespaced > securityfs files and how they will be deleted. How about this: ima: Move IMA securityfs files into ima_namespace or onto stack Move the IMA policy file's dentry into the ima_namespace for reuse by virtualized securityfs and for being able to remove the file from the filesystem using securityfs_remove(). Move the other files' dentries onto the stack since they are not needed outside the function where they are created in. Also, their cleanup is automatically handled by the filesystem upon umount of a virtualized secruityfs instance, so they don't need to be explicitly freed anymore. When moving the dentry 'ima_policy' into ima_namespace rename it to 'policy_dentry' to clarify its datatype and avoid a name clash with 'int ima_policy' from ima_policy.c. Stefan >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> security/integrity/ima/ima.h | 2 ++ >> security/integrity/ima/ima_fs.c | 32 ++++++++++++++++++-------------- >> 2 files changed, 20 insertions(+), 14 deletions(-) >> >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >> index 82b3f6a98320..224b09617c52 100644 >> --- a/security/integrity/ima/ima.h >> +++ b/security/integrity/ima/ima.h >> @@ -140,6 +140,8 @@ struct ima_namespace { >> struct mutex ima_write_mutex; >> unsigned long ima_fs_flags; >> int valid_policy; >> + >> + struct dentry *policy_dentry; > None of the other securityfs files are renamed. Why is "ima_policy" > being renamed to "policy_dentry"? If there is a need, it should be > documented in the patch description. > > thanks, > > Mimi > >> } __randomize_layout; >> extern struct ima_namespace init_ima_ns; >>
On Tue, 2022-01-18 at 15:12 -0500, Stefan Berger wrote: > On 1/13/22 15:28, Mimi Zohar wrote: > > Hi Stefan, > > > > Nobody refers to the IMA securityfs files as dentries. The Subject > > line is suppose to provide a hint about the patch. How about changing > > the "Subject" line to "ima: Move IMA securityfs files into > > ima_namespaces or onto stack". > > > > On Tue, 2022-01-04 at 12:04 -0500, Stefan Berger wrote: > >> From: Stefan Berger <stefanb@linux.ibm.com> > >> > >> Move the policy file dentry into the ima_namespace for reuse by > >> virtualized SecurityFS and for being able to remove it from > >> the filesystem. Move the other dentries onto the stack. > > Missing is an explanation why the other IMA securityfs files can be on > > the stack. Maybe start out by saying that the ns_ima_init securityfs > > files are never deleted. Then transition into the IMA namespaced > > securityfs files and how they will be deleted. > > How about this: > > ima: Move IMA securityfs files into ima_namespace or onto stack > > Move the IMA policy file's dentry into the ima_namespace for reuse by > virtualized securityfs and for being able to remove the file from the > filesystem using securityfs_remove(). How about "Move the IMA securityfs policy file ..." > Move the other files' dentries onto the stack since they are not needed How about "Move the other IMA securityfs files ..." > outside the function where they are created in. Also, their cleanup is > automatically handled by the filesystem upon umount of a virtualized > secruityfs instance, so they don't need to be explicitly freed anymore. > > When moving the dentry 'ima_policy' into ima_namespace rename it to > 'policy_dentry' to clarify its datatype and avoid a name clash with > 'int ima_policy' from ima_policy.c. To prevent namespace pollution, static variables need to be prefixed (e.g. "ima_"). This is not a concern with variables inside the ima_namespace structure. Why not just rename the variable "policy". Refer to the section on "Naming" in Documentation/process/coding- style.rst. thanks, Mimi > > > > >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > >> --- > >> security/integrity/ima/ima.h | 2 ++ > >> security/integrity/ima/ima_fs.c | 32 ++++++++++++++++++-------------- > >> 2 files changed, 20 insertions(+), 14 deletions(-) > >> > >> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > >> index 82b3f6a98320..224b09617c52 100644 > >> --- a/security/integrity/ima/ima.h > >> +++ b/security/integrity/ima/ima.h > >> @@ -140,6 +140,8 @@ struct ima_namespace { > >> struct mutex ima_write_mutex; > >> unsigned long ima_fs_flags; > >> int valid_policy; > >> + > >> + struct dentry *policy_dentry; > > None of the other securityfs files are renamed. Why is "ima_policy" > > being renamed to "policy_dentry"? If there is a need, it should be > > documented in the patch description. > > > > thanks, > > > > Mimi > > > >> } __randomize_layout; > >> extern struct ima_namespace init_ima_ns; > >>
On 1/18/22 15:42, Mimi Zohar wrote: > On Tue, 2022-01-18 at 15:12 -0500, Stefan Berger wrote: >> On 1/13/22 15:28, Mimi Zohar wrote: >>> Hi Stefan, >>> >>> Nobody refers to the IMA securityfs files as dentries. The Subject >>> line is suppose to provide a hint about the patch. How about changing >>> the "Subject" line to "ima: Move IMA securityfs files into >>> ima_namespaces or onto stack". >>> >>> On Tue, 2022-01-04 at 12:04 -0500, Stefan Berger wrote: >>>> From: Stefan Berger <stefanb@linux.ibm.com> >>>> >>>> Move the policy file dentry into the ima_namespace for reuse by >>>> virtualized SecurityFS and for being able to remove it from >>>> the filesystem. Move the other dentries onto the stack. >>> Missing is an explanation why the other IMA securityfs files can be on >>> the stack. Maybe start out by saying that the ns_ima_init securityfs >>> files are never deleted. Then transition into the IMA namespaced >>> securityfs files and how they will be deleted. >> How about this: >> >> ima: Move IMA securityfs files into ima_namespace or onto stack >> >> Move the IMA policy file's dentry into the ima_namespace for reuse by >> virtualized securityfs and for being able to remove the file from the >> filesystem using securityfs_remove(). > How about "Move the IMA securityfs policy file ..." > >> Move the other files' dentries onto the stack since they are not needed > How about "Move the other IMA securityfs files ..." > >> outside the function where they are created in. Also, their cleanup is >> automatically handled by the filesystem upon umount of a virtualized >> secruityfs instance, so they don't need to be explicitly freed anymore. >> >> When moving the dentry 'ima_policy' into ima_namespace rename it to >> 'policy_dentry' to clarify its datatype and avoid a name clash with >> 'int ima_policy' from ima_policy.c. > To prevent namespace pollution, static variables need to be prefixed > (e.g. "ima_"). This is not a concern with variables inside the > ima_namespace structure. Why not just rename the variable "policy". 'policy' is so generic. It can be the internal representation of the policy. > > Refer to the section on "Naming" in Documentation/process/coding- > style.rst. Hm, it cannot also be the point to work around the naming just to avoid it and come up with ambiguous names... 'policy_dentry' explains much better what it is but following this style guide that is then "Hungarian" notation, which is 'asinine.' What about 'policy_file'? Stefan > > thanks, > > Mimi > >> >> >>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >>>> --- >>>> security/integrity/ima/ima.h | 2 ++ >>>> security/integrity/ima/ima_fs.c | 32 ++++++++++++++++++-------------- >>>> 2 files changed, 20 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h >>>> index 82b3f6a98320..224b09617c52 100644 >>>> --- a/security/integrity/ima/ima.h >>>> +++ b/security/integrity/ima/ima.h >>>> @@ -140,6 +140,8 @@ struct ima_namespace { >>>> struct mutex ima_write_mutex; >>>> unsigned long ima_fs_flags; >>>> int valid_policy; >>>> + >>>> + struct dentry *policy_dentry; >>> None of the other securityfs files are renamed. Why is "ima_policy" >>> being renamed to "policy_dentry"? If there is a need, it should be >>> documented in the patch description. >>> >>> thanks, >>> >>> Mimi >>> >>>> } __randomize_layout; >>>> extern struct ima_namespace init_ima_ns; >>>> >
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index 82b3f6a98320..224b09617c52 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -140,6 +140,8 @@ struct ima_namespace { struct mutex ima_write_mutex; unsigned long ima_fs_flags; int valid_policy; + + struct dentry *policy_dentry; } __randomize_layout; extern struct ima_namespace init_ima_ns; diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 973bf17754b2..5b6404991b37 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -359,14 +359,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf, return result; } -static struct dentry *ima_dir; -static struct dentry *ima_symlink; -static struct dentry *binary_runtime_measurements; -static struct dentry *ascii_runtime_measurements; -static struct dentry *runtime_measurements_count; -static struct dentry *violations; -static struct dentry *ima_policy; - enum ima_fs_flags { IMA_FS_BUSY, }; @@ -436,8 +428,8 @@ static int ima_release_policy(struct inode *inode, struct file *file) ima_update_policy(ns); #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY) - securityfs_remove(ima_policy); - ima_policy = NULL; + securityfs_remove(ns->policy_dentry); + ns->policy_dentry = NULL; #elif defined(CONFIG_IMA_WRITE_POLICY) clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); #elif defined(CONFIG_IMA_READ_POLICY) @@ -454,8 +446,15 @@ static const struct file_operations ima_measure_policy_ops = { .llseek = generic_file_llseek, }; -int __init ima_fs_init(void) +static int __init ima_fs_ns_init(struct ima_namespace *ns) { + struct dentry *ima_dir; + struct dentry *ima_symlink = NULL; + struct dentry *binary_runtime_measurements = NULL; + struct dentry *ascii_runtime_measurements = NULL; + struct dentry *runtime_measurements_count = NULL; + struct dentry *violations = NULL; + ima_dir = securityfs_create_dir("ima", integrity_dir); if (IS_ERR(ima_dir)) return -1; @@ -492,20 +491,25 @@ int __init ima_fs_init(void) if (IS_ERR(violations)) goto out; - ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS, + ns->policy_dentry = securityfs_create_file("policy", POLICY_FILE_FLAGS, ima_dir, NULL, &ima_measure_policy_ops); - if (IS_ERR(ima_policy)) + if (IS_ERR(ns->policy_dentry)) goto out; return 0; out: + securityfs_remove(ns->policy_dentry); securityfs_remove(violations); securityfs_remove(runtime_measurements_count); securityfs_remove(ascii_runtime_measurements); securityfs_remove(binary_runtime_measurements); securityfs_remove(ima_symlink); securityfs_remove(ima_dir); - securityfs_remove(ima_policy); return -1; } + +int __init ima_fs_init(void) +{ + return ima_fs_ns_init(&init_ima_ns); +}