lkml.org 
[lkml]   [2022]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v8 10/19] ima: Implement hierarchical processing of file accesses
    On Tue, Jan 04, 2022 at 12:04:07PM -0500, Stefan Berger wrote:
    > From: Stefan Berger <stefanb@linux.ibm.com>
    >
    > 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@linux.ibm.com>
    > ---
    > 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.

    \
     
     \ /
      Last update: 2022-01-14 12:22    [W:6.604 / U:0.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site