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);
}
or sm similar.
