| Message ID | 20200623003236.830149-3-tyhicks@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support | expand |
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; > }
On 2020-06-25 15:33:33, Mimi Zohar wrote: > 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. That's effectively what's happening since the fsname, keyrings, and template pointers are being set to NULL, before exiting ima_lsm_copy_rule(), in the ima_rule_entry that's going to be freed. This patch is only introducing the function which can free all memory associated with a rule and is starting to use it in place that a rule entry is freed. Would you rather me introduce ima_free_rule() for the upcoming memory leak fixes in the series but not make use of it in ima_lsm_update_rule()? Tyler > > > > > return 0; > > }
On Thu, 2020-06-25 at 14:56 -0500, Tyler Hicks wrote: > On 2020-06-25 15:33:33, Mimi Zohar wrote: > > 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. > > That's effectively what's happening since the fsname, keyrings, and > template pointers are being set to NULL, before exiting > ima_lsm_copy_rule(), in the ima_rule_entry that's going to be freed. Ah, that clarified the reason for setting fsname, keyrings, ... to null before calling ima_free_rule. > > This patch is only introducing the function which can free all memory > associated with a rule and is starting to use it in place that a rule > entry is freed. > > Would you rather me introduce ima_free_rule() for the upcoming memory > leak fixes in the series but not make use of it in > ima_lsm_update_rule()? You could add a comment explaining the NULLs, but it might be clearer to keep the direct call to ima_lsm_free_rule(). Mimi
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); return 0; }
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> --- security/integrity/ima/ima_policy.c | 33 +++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)