Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <1593113613.27152.345.camel@linux.ibm.com>
Subject: Re: [PATCH 02/12] ima: Create a function to free a rule entry
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Tyler Hicks <tyhicks@linux.microsoft.com>,
        Dmitry Kasatkin <dmitry.kasatkin@gmail.com>
Cc:     James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>,
        Lakshmi Ramasubramanian <nramas@linux.microsoft.com>,
        Prakhar Srivastava <prsriva02@gmail.com>,
        linux-kernel@vger.kernel.org, linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org
Date:   Thu, 25 Jun 2020 15:33:33 -0400
In-Reply-To: <20200623003236.830149-3-tyhicks@linux.microsoft.com>
References: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
         <20200623003236.830149-3-tyhicks@linux.microsoft.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2020-06-22 at 19:32 -0500, Tyler Hicks wrote:
> There are several possible pieces of allocated memory in a rule entry.
> Create a function that can free all allocated memory for a given rule
> entry.
> 
> This patch introduces no functional changes but sets the groundwork for
> some memory leak fixes.
> 
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>

Having a function to release all memory associated with a policy rule
in general is a good idea.  However, in the case of the shallow copy,
we're not removing any IMA rules, just updating the LSM info.

There is an opportunity to transition from the builtin policy rules to
a custom IMA policy.  Afterwards IMA policy rules may only be
appended.

An IMA custom policy based on LSM info may be loaded prior to the LSM
policy.  These LSM based rules are inactive until the corresponding
LSM rule is loaded.  In some environments, LSM policies are loaded and
removed frequently.  The IMA rules themselves are not removed, just
the LSM info is updated to reflect the current LSM info.

> ---
>  security/integrity/ima/ima_policy.c | 33 +++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 236a731492d1..1320333201c6 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -261,6 +261,27 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
>  		security_filter_rule_free(entry->lsm[i].rule);
>  		kfree(entry->lsm[i].args_p);
>  	}
> +}
> +
> +static void ima_free_rule(struct ima_rule_entry *entry)
> +{
> +	if (!entry)
> +		return;
> +
> +	/*
> +	 * entry->template->fields may be allocated in ima_parse_rule() but that
> +	 * reference is owned by the corresponding ima_template_desc element in
> +	 * the defined_templates list and cannot be freed here
> +	 */
> +
> +	/*
> +	 * When freeing newly added ima_rule_entry members, consider if you
> +	 * need to disown any references after the shallow copy in
> +	 * ima_lsm_copy_rule()
> +	 */
> +	kfree(entry->fsname);
> +	kfree(entry->keyrings);
> +	ima_lsm_free_rule(entry);
>  	kfree(entry);
>  }
>  
> @@ -298,10 +319,18 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
>  			pr_warn("rule for LSM \'%s\' is undefined\n",
>  				(char *)entry->lsm[i].args_p);
>  	}
> +
> +	/* Disown all references that were shallow copied */
> +	entry->fsname = NULL;
> +	entry->keyrings = NULL;
> +	entry->template = NULL;
>  	return nentry;
>  
>  out_err:
> -	ima_lsm_free_rule(nentry);
> +	nentry->fsname = NULL;
> +	nentry->keyrings = NULL;
> +	nentry->template = NULL;
> +	ima_free_rule(nentry);

>  	return NULL;
>  }
>  
> @@ -315,7 +344,7 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
>  
>  	list_replace_rcu(&entry->list, &nentry->list);
>  	synchronize_rcu();
> -	ima_lsm_free_rule(entry);
> +	ima_free_rule(entry);

This should only update the LSM info, nothing else.

>  
>  	return 0;
>  }