Messages in this thread Patches in this message | | | From | "Woodhouse, David" <> | Subject | Re: [PATCH] params: fix potential memory leak in add_sysfs_param() | Date | Wed, 20 Aug 2014 22:17:48 +0000 |
| |
On Thu, 2014-08-21 at 07:35 +0930, Rusty Russell wrote: > > Above this: > if (!mk->mp) { > num = 0; > attrs = NULL; > } else { > num = mk->mp->num; > attrs = mk->mp->grp.attrs; > } > > So, attrs is just a temporary: either NULL (doesn't need freeing), or > is the old mk->mp->grp.attrs ptr.
Except that in the failure case we *free* the old mk->mp and never free mk->mp->grp.attrs so it *is* indeed lost.
A simpler version of Arjun's patch might look like this:
diff --git a/kernel/params.c b/kernel/params.c index 34f5270..f9459bc 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -613,7 +613,6 @@ static __modinit int add_sysfs_param(struct module_kobject *mk, sizeof(*mk->mp) + sizeof(mk->mp->attrs[0]) * (num+1), GFP_KERNEL); if (!new) { - kfree(attrs); err = -ENOMEM; goto fail; } @@ -653,7 +652,10 @@ static __modinit int add_sysfs_param(struct module_kobject *mk, fail_free_new: kfree(new); fail: - mk->mp = NULL; + if (mk->mp) { + kfree(mk->mp->grp.attrs); + mk->mp = NULL; + } return err; }
But as I suggested in my previous response, a *better* fix might look like this:
diff --git a/kernel/params.c b/kernel/params.c index 34f5270..cdab9d4 100644 --- a/kernel/params.c +++ b/kernel/params.c @@ -595,7 +595,7 @@ static __modinit int add_sysfs_param(struct module_kobject *mk, { struct module_param_attrs *new; struct attribute **attrs; - int err, num; + int num; /* We don't bother calling this with invisible parameters. */ BUG_ON(!kp->perm); @@ -612,18 +612,19 @@ static __modinit int add_sysfs_param(struct module_kobject *mk, new = krealloc(mk->mp, sizeof(*mk->mp) + sizeof(mk->mp->attrs[0]) * (num+1), GFP_KERNEL); - if (!new) { - kfree(attrs); - err = -ENOMEM; - goto fail; - } + if (!new) + return -ENOMEM; + /* Despite looking like the typical realloc() bug, this is safe. - * We *want* the old 'attrs' to be freed either way, and we'll store - * the new one in the success case. */ + * In the failure case, the old 'attrs' is still in new->grp.attrs + * and will live on there. */ attrs = krealloc(attrs, sizeof(new->grp.attrs[0])*(num+2), GFP_KERNEL); if (!attrs) { - err = -ENOMEM; - goto fail_free_new; + /* This is in a larger kmalloc allocation than before but + * otherwise entirely unchanged. We've failed to add the + * new param but the existing ones are still there. */ + mk->mp = new; + return -ENOMEM; } /* Sysfs wants everything zeroed. */ @@ -649,12 +650,6 @@ static __modinit int add_sysfs_param(struct module_kobject *mk, mk->mp = new; return 0; - -fail_free_new: - kfree(new); -fail: - mk->mp = NULL; - return err; } #ifdef CONFIG_MODULES
-- David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation [unhandled content-type:application/x-pkcs7-signature] | |