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>] [day] [month] [year] [list]

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

2009/7/25 Xiaotian Feng <xtfeng@...il.com>:
>
>
> On Sat, Jul 25, 2009 at 11:38 AM, Ming Lei <tom.leiming@...il.com> wrote:
>>
>> 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?
>
>
> No, take a look at kobject_put code, you will see if foo->kobj is NULL,
> kobject_put will do nothing.
> So it's safe for double kobject_put on the register failed path.

You are wrong, foo->kobj  is __not__ NULL.

Can the kobject_init_and_add update the passed pointer of foo->kobj?

No, it can not. Even kobject_init_and_add returns failure, the pointer
of kobject passed does not change, so kobject_put() still can see the
old pointer.

Thanks.
-- 
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/