Re: [PATCH] security: smack: fix memleak in smk_write_rules_list()

From: Casey Schaufler
Date: Fri Jun 28 2013 - 15:40:21 EST


On 6/19/2013 7:08 AM, Tomasz Stanislawski wrote:
> >From 8497987bedf8821db3dce47a6205dfce2b0895c5 Mon Sep 17 00:00:00 2001
> From: Tomasz Stanislawski <t.stanislaws@xxxxxxxxxxx>
> Date: Thu, 6 Jun 2013 09:30:50 +0200
> Subject: [PATCH] security: smack: fix memleak in smk_write_rules_list()
>
> The smack_parsed_rule structure is allocated. If a rule is successfully
> installed then the last reference to the object is lost. This patch fixes this
> leak. Moreover smack_parsed_rule is allocated on stack because it no longer
> needed ofter smk_write_rules_list() is finished.
>
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@xxxxxxxxxxx>

I will add this patch to the smack-next tree.

Acked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>

> ---
> security/smack/smackfs.c | 30 ++++++++++--------------------
> 1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 53a08b8..08aebc2 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -446,7 +446,7 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
> struct mutex *rule_lock, int format)
> {
> struct smack_known *skp;
> - struct smack_parsed_rule *rule;
> + struct smack_parsed_rule rule;
> char *data;
> int datalen;
> int rc = -EINVAL;
> @@ -478,49 +478,39 @@ static ssize_t smk_write_rules_list(struct file *file, const char __user *buf,
> goto out;
> }
>
> - rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> - if (rule == NULL) {
> - rc = -ENOMEM;
> - goto out;
> - }
> -
> if (format == SMK_LONG_FMT) {
> /*
> * Be sure the data string is terminated.
> */
> data[count] = '\0';
> - if (smk_parse_long_rule(data, rule, 1, 0))
> - goto out_free_rule;
> + if (smk_parse_long_rule(data, &rule, 1, 0))
> + goto out;
> } else if (format == SMK_CHANGE_FMT) {
> data[count] = '\0';
> - if (smk_parse_long_rule(data, rule, 1, 1))
> - goto out_free_rule;
> + if (smk_parse_long_rule(data, &rule, 1, 1))
> + goto out;
> } else {
> /*
> * More on the minor hack for backward compatibility
> */
> if (count == (SMK_OLOADLEN))
> data[SMK_OLOADLEN] = '-';
> - if (smk_parse_rule(data, rule, 1))
> - goto out_free_rule;
> + if (smk_parse_rule(data, &rule, 1))
> + goto out;
> }
>
>
> if (rule_list == NULL) {
> load = 1;
> - skp = smk_find_entry(rule->smk_subject);
> + skp = smk_find_entry(rule.smk_subject);
> rule_list = &skp->smk_rules;
> rule_lock = &skp->smk_rules_lock;
> }
>
> - rc = smk_set_access(rule, rule_list, rule_lock, load);
> - if (rc == 0) {
> + rc = smk_set_access(&rule, rule_list, rule_lock, load);
> + if (rc == 0)
> rc = count;
> - goto out;
> - }
>
> -out_free_rule:
> - kfree(rule);
> out:
> kfree(data);
> return rc;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/