linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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
Subject: Re: [PATCH 02/12] ima: Create a function to free a rule entry
Date: Thu, 25 Jun 2020 15:33:33 -0400	[thread overview]
Message-ID: <1593113613.27152.345.camel@linux.ibm.com> (raw)
In-Reply-To: <20200623003236.830149-3-tyhicks@linux.microsoft.com>

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;
>  }


  reply	other threads:[~2020-06-25 19:33 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-23  0:32 [PATCH 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support Tyler Hicks
2020-06-23  0:32 ` [PATCH 01/12] ima: Have the LSM free its audit rule Tyler Hicks
2020-06-23  0:55   ` Casey Schaufler
2020-06-23  3:04     ` Tyler Hicks
2020-06-23 23:04   ` Tyler Hicks
2020-06-25 19:41   ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 02/12] ima: Create a function to free a rule entry Tyler Hicks
2020-06-25 19:33   ` Mimi Zohar [this message]
2020-06-25 19:56     ` Tyler Hicks
2020-06-25 20:32       ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 03/12] ima: Free the entire rule when deleting a list of rules Tyler Hicks
2020-06-25 21:05   ` Mimi Zohar
2020-06-25 21:07   ` Mimi Zohar
2020-06-25 21:08     ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 04/12] ima: Free the entire rule if it fails to parse Tyler Hicks
2020-06-23  0:32 ` [PATCH 05/12] ima: Fail rule parsing when buffer hook functions have an invalid action Tyler Hicks
2020-06-25 21:51   ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 06/12] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond Tyler Hicks
2020-06-25 21:53   ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 07/12] ima: Fail rule parsing when the KEY_CHECK " Tyler Hicks
2020-06-23  0:32 ` [PATCH 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements Tyler Hicks
2020-06-25 21:18   ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 09/12] ima: Use correct type for " Tyler Hicks
2020-06-25 21:20   ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 10/12] ima: Move validation of the keyrings conditional into ima_validate_rule() Tyler Hicks
2020-06-25 19:50   ` Tyler Hicks
2020-06-25 20:46     ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 11/12] ima: Use the common function to detect LSM conditionals in a rule Tyler Hicks
2020-06-25 22:45   ` Mimi Zohar
2020-06-23  0:32 ` [PATCH 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function Tyler Hicks
2020-06-25 22:56   ` Mimi Zohar
2020-06-25 22:59     ` Tyler Hicks

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1593113613.27152.345.camel@linux.ibm.com \
    --to=zohar@linux.ibm.com \
    --cc=dmitry.kasatkin@gmail.com \
    --cc=jmorris@namei.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=nramas@linux.microsoft.com \
    --cc=prsriva02@gmail.com \
    --cc=serge@hallyn.com \
    --cc=tyhicks@linux.microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).