Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 2 Jun 2022 08:58:28 +0200
From:   =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>
To:     Tyrel Datwyler <tyreld@linux.ibm.com>
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Paul Mackerras <paulus@samba.org>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Nathan Lynch <nathanl@linux.ibm.com>,
        Laurent Dufour <ldufour@linux.ibm.com>,
        Daniel Henrique Barboza <danielhb413@gmail.com>,
        David Gibson <david@gibson.dropbear.id.au>,
        Andrew Morton <akpm@linux-foundation.org>,
        David Hildenbrand <david@redhat.com>,
        Ohhoon Kwon <ohoono.kwon@samsung.com>,
        "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>,
        YueHaibing <yuehaibing@huawei.com>, devicetree@vger.kernel.org,
        Steen Hegelund <steen.hegelund@microchip.com>,
        linux-kernel@vger.kernel.org, Lizhi Hou <lizhi.hou@xilinx.com>,
        Allan Nielsen <allan.nielsen@microchip.com>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Bjorn Helgaas <helgaas@kernel.org>,
        linuxppc-dev@lists.ozlabs.org,
        Horatiu Vultur <horatiu.vultur@microchip.com>
Subject: Re: [PATCH v2 2/4] of: dynamic: add of_property_alloc() and
 of_property_free()
Message-ID: <20220602085828.2138554a@fixe.home>
In-Reply-To: <4b92277e-5133-2362-8d3a-fa82b0c7a045@linux.ibm.com>
References: <20220601081801.348571-1-clement.leger@bootlin.com>
        <20220601081801.348571-3-clement.leger@bootlin.com>
        <4b92277e-5133-2362-8d3a-fa82b0c7a045@linux.ibm.com>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Le Wed, 1 Jun 2022 15:32:29 -0700,
Tyrel Datwyler <tyreld@linux.ibm.com> a =C3=A9crit :

> >  /**
> > - * __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;
> > + =20
>=20
> This looks wrong to me. From what I understand the value data is allocate=
d as
> trailing memory that is part of the property allocation itself. (ie. prop=
 =3D
> kzalloc(sizeof(*prop) + len, allocflags)). So, kfree(prop) should also ta=
ke 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.

>=20
> Also, this condition will always fail. You explicitly set prop->value =3D=
 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).

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


--=20
Cl=C3=A9ment L=C3=A9ger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com