On Fri, 2021-12-10 at 10:32 -0500, Stefan Berger wrote: > On 12/10/21 10:26, Mimi Zohar wrote: > > On Fri, 2021-12-10 at 09:26 -0500, James Bottomley wrote: > >> On Fri, 2021-12-10 at 09:17 -0500, Stefan Berger wrote: > >>> On 12/10/21 08:02, Mimi Zohar wrote: > >>>> On Fri, 2021-12-10 at 07:40 -0500, Stefan Berger wrote: > >>>>> On 12/10/21 07:09, Mimi Zohar wrote: > >>>>>> On Fri, 2021-12-10 at 12:49 +0100, Christian Brauner wrote: > >>>>>>>> There's still the problem that if you write the policy, > >>>>>>>> making the file disappear then unmount and remount > >>>>>>>> securityfs it will come back. My guess for fixing this is > >>>>>>>> that we only stash the policy file reference, > >>>>>>>> create it if NULL but then set the pointer to PTR_ERR(- > >>>>>>>> EINVAL) or something and refuse to create it for that > >>>>>>>> value. > >>>>>>> Some sort of indicator that gets stashed in struct ima_ns > >>>>>>> that the file does not get recreated on consecutive mounts. > >>>>>>> That shouldn't be hard to fix. > >>>>>> The policy file disappearing is for backwards compatibility, > >>>>>> prior to being able to extend the custom policy. For embedded > >>>>>> usecases, allowing the policy to be written exactly once might > >>>>>> makes sense. Do we really want/need to continue to support > >>>>>> removing the policy in namespaces? > >>>>> I don't have an answer but should the behavior for the same > >>>>> #define in this case be different for host and namespaces? Or > >>>>> should we just 'select IMA_WRITE_POLICY and IMA_READ_POLICY' when > >>>>> IMA_NS is selected? > >>>> The latter option sounds good. Being able to analyze the namespace > >>>> policy is really important. > >>> Ok, I will adjust the Kconfig for this then. This then warrants the > >>> question whether to move the dentry into the ima_namespace. The > >>> current code looks like this. > >>> > >>> #if !defined(CONFIG_IMA_WRITE_POLICY) && > >>> !defined(CONFIG_IMA_READ_POLICY) > >>> securityfs_remove(ns->policy_dentry); > >>> ns->policy_dentry = NULL; > >>> ns->policy_dentry_removed = true; > >>> #elif defined(CONFIG_IMA_WRITE_POLICY) > >>> > >>> With IMA_NS selecting IMA_WRITE_POLICY and IMA_READ_POLICY the above > >>> wouldn't be necessary anymore but I find it 'cleaner' to still have > >>> the dentry isolated rather than it being a global static as it was > >>> before... > >> This is really, really why you don't want the semantics inside the > >> namespace to differ from those outside, because it creates confusion > >> for the people reading the code, especially with magically forced > >> config options like this. I'd strongly suggest you either keep the > >> semantic in the namespace or eliminate it entirely. > >> > >> If you really, really have to make the namespace behave differently, > >> then use global variables and put a big comment on that code saying it > >> can never be reached once CONFIG_IMA_NS is enabled. > > The problem seems to be with removing the securityfs policy file. > > Instead of removing it, just make it inacessible for the "if > > !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)" > > case. > > So we would then leave it up to the one building the kernel to select > the proper compile time options (suggested ones being IMA_WRITE_POLICY > and IMA_READ_POLICY being enabled?) and behavior of host and IMA > namespace is then the same per those options? Removing the file didn't > seem the problem to me but more like whether the host should ever behave > differently from the namespace. You proposed "select IMA_WRITE_POLICY and IMA_READ_POLICY'" when IMA_NS is selected. At least IMA_READ_POLICY should be enabled for namespaces. In addition, if removing the securityfs file after a custom policy is loaded complicates namespacing, then don't remove it. thanks, Mimi
