Re: [PATCH v8 10/19] ima: Implement hierarchical processing of file accesses

[Stefan Berger]

On 1/14/22 06:21, Christian Brauner wrote:
> On Tue, Jan 04, 2022 at 12:04:07PM -0500, Stefan Berger wrote:
> > From: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> >
> > Implement hierarchical processing of file accesses in IMA namespaces by
> > walking the list of user namespaces towards the root. This way file
> > accesses can be audited in an IMA namespace and also be evaluated against
> > the IMA policies of parent IMA namespaces.
> >
> > __process_measurement() returns either 0 or -EACCES. For hierarchical
> > processing remember the -EACCES returned by this function but continue
> > to the parent user namespace. At the end either return 0 or -EACCES
> > if an error occurred in one of the IMA namespaces.
> >
> > Currently the ima_ns pointer of the user_namespace is always NULL except
> > at the init_user_ns, so test ima_ns for NULL pointer and skip the call to
> > __process_measurement() if it is NULL. Once IMA namespacing is fully
> > enabled, the pointer may also be NULL due to late initialization of the
> > IMA namespace.
> >
> > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > ---
> >   include/linux/ima.h               |  6 +++++
> >   security/integrity/ima/ima_main.c | 37 +++++++++++++++++++++++++++----
> >   2 files changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index b6ab66a546ae..fcee2a51bb87 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -65,6 +65,12 @@ static inline const char * const *arch_get_ima_policy(void)
> >   }
> >   #endif
> >   
> > +static inline struct user_namespace
> > +*ima_ns_to_user_ns(struct ima_namespace *ns)
> > +{
> > +	return current_user_ns();
> > +}
> > +
> >   #else
> >   static inline enum hash_algo ima_get_current_hash_algo(void)
> >   {
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 621685d4eb95..51b0ef1cebbe 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -200,10 +200,10 @@ void ima_file_free(struct file *file)
> >   	ima_check_last_writer(iint, inode, file);
> >   }
> >   
> > -static int process_measurement(struct ima_namespace *ns,
> > -			       struct file *file, const struct cred *cred,
> > -			       u32 secid, char *buf, loff_t size, int mask,
> > -			       enum ima_hooks func)
> > +static int __process_measurement(struct ima_namespace *ns,
> > +				 struct file *file, const struct cred *cred,
> > +				 u32 secid, char *buf, loff_t size, int mask,
> > +				 enum ima_hooks func)
> >   {
> >   	struct inode *inode = file_inode(file);
> >   	struct integrity_iint_cache *iint = NULL;
> > @@ -395,6 +395,35 @@ static int process_measurement(struct ima_namespace *ns,
> >   	return 0;
> >   }
> >   
> > +static int process_measurement(struct ima_namespace *ns,
> > +			       struct file *file, const struct cred *cred,
> > +			       u32 secid, char *buf, loff_t size, int mask,
> > +			       enum ima_hooks func)
> > +{
> > +	struct user_namespace *user_ns = ima_ns_to_user_ns(ns);
> > +	int ret = 0;
> > +
> > +	while (user_ns) {
> > +		ns = ima_ns_from_user_ns(user_ns);
> > +		if (ns) {
> > +			int rc;
> > +
> > +			rc = __process_measurement(ns, file, cred, secid, buf,
> > +						   size, mask, func);
> > +			switch (rc) {
> > +			case -EACCES:
> > +				/* return this error at the end but continue */
> > +				ret = -EACCES;
> > +				break;
> This seems risky. Every error not -EACCES will be counted as a success.
> It doesn't look like __process_measurement() will return anything else
> but I would still place a WARN_ON() or WARN_ON_ONCE() in there to make
> that assumption explicit.
>
> Right now it looks like your only error condition is -EACCES and non-ima
> cracks like me need to read through __process_measurement() to figure
> out that that's ok. With a WARN_ON* in there I'd not have needed to bother.
>
> switch (rc) {
> case -EACCES:
> 	/* return this error at the end but continue */
> 	ret = -EACCES;
> 	break
> default:
> 	WARN_ON_ONCE(true);
             ret = -EINVAL;
> }
>
> or sm similar.


Agreed. To be on the safe side I would add a ret = -EINVAL to it for the 
unhandled case as shown above.