linux-kernel - Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and of_property

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

Message-ID: <04d9e84f-dc35-55f1-efd5-81114a6784cf@linux.ibm.com>
Date:   Thu, 2 Jun 2022 11:10:07 -0700
From:   Tyrel Datwyler <tyreld@...ux.ibm.com>
To:     Clément Léger <clement.leger@...tlin.com>
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Nathan Lynch <nathanl@...ux.ibm.com>,
        Laurent Dufour <ldufour@...ux.ibm.com>,
        Daniel Henrique Barboza <danielhb413@...il.com>,
        David Gibson <david@...son.dropbear.id.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...hat.com>,
        Ohhoon Kwon <ohoono.kwon@...sung.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
        YueHaibing <yuehaibing@...wei.com>, devicetree@...r.kernel.org,
        Steen Hegelund <steen.hegelund@...rochip.com>,
        linux-kernel@...r.kernel.org, Lizhi Hou <lizhi.hou@...inx.com>,
        Allan Nielsen <allan.nielsen@...rochip.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Bjorn Helgaas <helgaas@...nel.org>,
        linuxppc-dev@...ts.ozlabs.org,
        Horatiu Vultur <horatiu.vultur@...rochip.com>
Subject: Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and
 of_property_free()

On 6/1/22 23:58, Clément Léger wrote:
> Le Wed, 1 Jun 2022 15:32:29 -0700,
> Tyrel Datwyler <tyreld@...ux.ibm.com> a écrit :
> 
>>>  /**
>>> - * __of_prop_dup - Copy a property dynamically.
>>> - * @prop:	Property to copy
>>> + * of_property_free - Free a property allocated dynamically.
>>> + * @prop:	Property to be freed
>>> + */
>>> +void of_property_free(const struct property *prop)
>>> +{
>>> +	if (!of_property_check_flag(prop, OF_DYNAMIC))
>>> +		return;
>>> +  
>>
>> This looks wrong to me. From what I understand the value data is allocated as
>> trailing memory that is part of the property allocation itself. (ie. prop =
>> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also take care
>> of the trailing value data. Calling kfree(prop->value) is bogus since
>> prop->value wasn't dynamically allocated on its own.
> 
> kfree(prop->value) is only called if the value is not the trailing data
> of the property so I don't see what is wrong there. In that case, only
> kfree(prop) is called.

Right, Rob clarified for me in the v1 patch.

> 
>>
>> Also, this condition will always fail. You explicitly set prop->value = prop + 1
>> in alloc.
> 
> The user that did allocated the property might want to provide its own
> "value". In that case, prop->value would be overwritten by the user
> allocated value and thus the check would be true, hence calling
> kfree(prop->value).

So, that was the part I was missing. I think a comment would be helpful so its
clear value can be either trailing or user assigned.

-Tyrel

> 
>>
>> Maybe I need to go back and look at v1 again.
>>
>> -Tyrel
>>
>>> +	if (prop->value != prop + 1)
>>> +		kfree(prop->value);
>>> +
>>> +	kfree(prop->name);
>>> +	kfree(prop);
>>> +}
>>> +EXPORT_SYMBOL(of_property_free);
>>> +
> 
>