linux-kernel - Re: [PATCH] lib/kobject: put kobject if kobject_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <d82e647a0907242038l460c01b0ud51a7265f70d6c2e@mail.gmail.com>
Date:	Sat, 25 Jul 2009 11:38:07 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Xiaotian Feng <dfeng@...hat.com>
Cc:	gregkh@...e.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] lib/kobject: put kobject if kobject_add_internal fails

2009/7/24 Xiaotian Feng <dfeng@...hat.com>:
> The proper way to use kobject_init_and_add should be:
>
>        retval = kobject_init_and_add(&foo->kobj, &foo_ktype, NULL, "%s", name);
>        if (retval) {
>                kobject_put(&foo->kobj); (*)
>                return NULL;
>        }
>

Yes, you are correct.

> kobject_init_and_add calls kobject_add_vargs finally, kobject_add_vargs is divided
> into two parts: kobject_set_name_vargs and kobject_add_internal. Both the two parts
> may return an error. If the error is came from kobject_add_internal, this means
> kobject_set_name_vargs already alloc memory for kobj->name.
>
> So if caller forgets to use kobject_put when this kind of error occurs, the memory
> for kobj->name leaks. Unfortunately, most of the callers is forgotten to use
> kobject_put in this rare situation, grep kobject_init_and_add in kernel source code,
> there are 20+ files forgotten this. So I'd prefer to fix this in lib/kobject, not
> the whole 20+ files.

No, you should fix the 20+ files instead of lib/kobject. One rule should be:

             One who allocated kobject should free the kobject,
instead of others.

Image you have allocated a object, and call some .init function to
initialize it,
but .init frees the object due to some exception, it is very ugly and very prone
to access of the freed object.

Your patch may lead to much oops if the drivers use the proper way of
kobject_init_and_add:

        retval = kobject_init_and_add(&foo->kobj, &foo_ktype, NULL, "%s", name);
        if (retval) {
                kobject_put(&foo->kobj);      /*foo->kobj has been freed, oops*/
                return NULL;
        }

Right?

>
> Signed-off-by: Xiaotian Feng <dfeng@...hat.com>
> ---
>  lib/kobject.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/lib/kobject.c b/lib/kobject.c
> index b512b74..e6e6e03 100644
> --- a/lib/kobject.c
> +++ b/lib/kobject.c
> @@ -200,6 +200,7 @@ static int kobject_add_internal(struct kobject *kobj)
>                        printk(KERN_ERR "%s failed for %s (%d)\n",
>                               __func__, kobject_name(kobj), error);
>                dump_stack();
> +               kobject_put(kobj);
>        } else
>                kobj->state_in_sysfs = 1;
>
> @@ -657,7 +658,6 @@ struct kobject *kobject_create_and_add(const char *name, struct kobject *parent)
>        if (retval) {
>                printk(KERN_WARNING "%s: kobject_add error: %d\n",
>                       __func__, retval);
> -               kobject_put(kobj);
>                kobj = NULL;
>        }
>        return kobj;

NAK.

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