Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <e3c2e050-ce9b-c45d-6918-44384f8d10f6@gmx.de>
Date:   Mon, 25 Sep 2023 16:26:00 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [External] [PATCH] platform/x86: think-lmi: Fix reference leak
Content-Language: en-US
To:     Mark Pearson <markpearson@lenovo.com>
Cc:     hdegoede@redhat.com, markgross@kernel.org,
        ilpo.jarvinen@linux.intel.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230923204154.86815-1-W_Armin@gmx.de>
 <8e1592dd-7e95-20a6-15e7-f21798af3d37@lenovo.com>
From:   Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <8e1592dd-7e95-20a6-15e7-f21798af3d37@lenovo.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:CPIN7LPOP3M=;q02l1TqudyDj+/HE+p0zWCwrvxt
 6m1vj2Xmo0fXR1QFDCyjDbEVp9I6x0UmuaVCrP9pxuH2mIbXJOXvqtkh8yBbc1xYJiDuvNe5c
 YIHZ9OcokvPLlTT/nqlbkR0PkvV8v6UIPMimB/rdFCcDnvp8qWcLxSIbdJsrh960u7c1SErle
 EE76FB33RwZUON0jrfqKHBM5V00tufFokUI8gDwgkPUB3t2e2snwROSN1ujnPDFfW+uU4yPh8
 nOykyGL85RBb9An/6mGP+vYCq2a6xUCbmTG08PPmDaaE9EbBgvYrzq6x8iVfmGzbzDbQfuTBh
 3vqSog01k+Q87A2JY/HN5LV5FkwckOmUNGfS4gA2qNy7Wt5nII+4x6wFOhI1MxqBFujcJIzNn
 JRhjv00dl5v/1b8aCXhuwSTknoU+aD00TrhUKP0xJx7dJd+JA3pXJe1WpVSRSiJ6+GwN9Asgt
 Jlrl6lGRWSaeiseLAvD6TzZ268cFs7zahd3kaIPhTsFU0i18Ky51aLkCKXfWEhAoL/rCy3uIU
 roVvcTb/E6zJHhra2ztvfg27NIysbUhBO7Y8Z7C0YO+lMskQLp1K3myG/w6Gt8wu3BY/wIFb7
 jFXn1uu48YuO48ipIbhRWKz22/duPuBZF8P1FfMfY7Li0oICymvptpzrWjAqIAE6JQcRj+9Ns
 VDeBSwIUQ8yBInIvyI7CHrTqxTU8Y3zGEgb0eUPphW2vSZFfEOm1tRZUzWGKtvfdk26XzJPAF
 8Snt+XTo1O7X6+t2ZTaxkNMxXbXlpGRroT1ecf687Jjngnmd1AR4IXC1n6865f4uSC4mLB8ym
 GLM+bm0jd/rTFyVwKO2kAdbApugnaEe97ilkNdzhkfNgOPKlxilcppmEw4YPlv1ru8lu1EaxB
 1G9IgjNgyi09gup/jktPtcpsU2ssBi0k9EaugJgHV5kJuJejYCO6iZjiAB55lto35Zyp3aAFR
 RoyqhW6RMgIfsObueirczjSOgcI=
Precedence: bulk

Am 25.09.23 um 16:00 schrieb Mark Pearson:

> Thanks Armin,
>
> On 9/23/23 16:41, Armin Wolf wrote:
>> If a duplicate attribute is found using kset_find_obj(), a reference
>> to that attribute is returned which needs to be disposed accordingly
>> using kobject_put(). Move the setting name validation into a separate
>> function to allow for this change without having to duplicate the
>> cleanup code for this setting.
>> As a side note, a very similar bug was fixed in
>> commit 7295a996fdab ("platform/x86: dell-sysman: Fix reference leak"),
>> so it seems that the bug was copied from that driver.
>>
>> Compile-tested only.
>>
>> Fixes: 1bcad8e510b2 ("platform/x86: think-lmi: Fix issues with
>> duplicate attributes")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> =C2=A0 drivers/platform/x86/think-lmi.c | 24 ++++++++++++++++++++----
>> =C2=A0 1 file changed, 20 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c
>> b/drivers/platform/x86/think-lmi.c
>> index 4be6f28d4600..3a396b763c49 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -1344,6 +1344,24 @@ static void tlmi_release_attr(void)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kset_unregister(tlmi_priv.authentication=
_kset);
>> =C2=A0 }
>>
>> +static int tlmi_validate_setting_name(struct kset *attribute_kset,
>> char *name)
>> +{
>> +=C2=A0=C2=A0=C2=A0 struct kobject *duplicate;
>> +
>> +=C2=A0=C2=A0=C2=A0 if (!strcmp(name, "Reserved"))
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -EINVAL;
>> +
>> +=C2=A0=C2=A0=C2=A0 duplicate =3D kset_find_obj(attribute_kset, name);
>> +=C2=A0=C2=A0=C2=A0 if (duplicate) {
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_debug("Duplicate attribu=
te name found - %s\n", name);
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* kset_find_obj() returns =
a reference */
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kobject_put(duplicate);
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return -EBUSY;
>> +=C2=A0=C2=A0=C2=A0 }
>> +
>> +=C2=A0=C2=A0=C2=A0 return 0;
>> +}
>> +
>> =C2=A0 static int tlmi_sysfs_init(void)
>> =C2=A0 {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int i, ret;
>> @@ -1372,10 +1390,8 @@ static int tlmi_sysfs_init(void)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 continue;
>>
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* check for dup=
licate or reserved values */
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (kset_find_obj(tlmi_priv=
.attribute_kset,
>> tlmi_priv.setting[i]->display_name) ||
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 !st=
rcmp(tlmi_priv.setting[i]->display_name, "Reserved")) {
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 pr_=
debug("duplicate or reserved attribute name found -
>> %s\n",
>> -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0 tlmi_priv.setting[i]->display_name);
>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (tlmi_validate_setting_n=
ame(tlmi_priv.attribute_kset,
>> + tlmi_priv.setting[i]->display_name) < 0) {
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 kfree(tlmi_priv.setting[i]->possible_values);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 kfree(tlmi_priv.setting[i]);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 tlmi_priv.setting[i] =3D NULL;
>> --
>> 2.39.2
>>
>
> Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>
> Do you have any reports of our platforms where these are seen? If so
> I'd like to get it fixed in FW too (and I can get it tested on HW if
> that helps)
>
> Mark
>
No, i do not have any reports regarding this issue. I stumbled upon this b=
ug by chance, after fixing a similar bug in dell-wmi-sysman.
I suspect that the bug was copied from there to the other drivers, like th=
ink-lmi and hp-bioscfg.

Armin Wolf