linux-kernel - Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in hp_add_other

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 for Android: free password hash cracker in your pocket

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

Message-ID: <fb97e3ea-1bee-4d7d-a8d4-dd76107f75ef@oracle.com>
Date:   Sat, 11 Nov 2023 01:28:25 +0530
From:   Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc:     Jorge Lopez <jorge.lopez2@...com>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Gross <markgross@...nel.org>,
        Thomas Weißschuh <linux@...ssschuh.net>,
        platform-driver-x86@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>, dan.carpenter@...aro.org,
        kernel-janitors@...r.kernel.org, error27@...il.com,
        vegard.nossum@...cle.com, darren.kenny@...cle.com,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2 4/4] platform/x86: hp-bioscfg: Fix error handling in
 hp_add_other_attributes()

Hi Ilpo,

On 10/11/23 8:14 pm, Ilpo Järvinen wrote:
> On Fri, 10 Nov 2023, Harshit Mogalapalli wrote:
> 

Thanks for the review.

> This changelog needs to be rewritten, it contains multiple errors. I
> suppose even this patch could be split into two but I'll not be too picky
> here if you insist on fixing them in the same patch.
> 

Any thoughts on how to split this into two patches ?

I thought of fixing memory leak in separate patch, but that would add 
more code which should be removed when we move kobject_put() to the end.

>> We have two issues:
>> 1. Memory leak of 'attr_name_kobj' in the error handling path.
> 
> True, but not specific enough to be useful.
> 

Should I mention something like:

'attr_name_kobj' is allocated using kzalloc, but on all the error paths 
we don't free it, hence we have a memory leak.

>> 2. When kobject_init_and_add() fails on every subsequent error path call
>>     kobject_put() to cleanup.
> 
> This makes no sense. The only case when there old code had no issue is
> "when kobject_init_and_add() fails" but now your wording claims it to be
> source of problem. Please rephrase this.
> 

Does this look better:

kobject_put() must be called to cleanup memory associated with the 
object if kobject_init_and_add() returns an error , before this patch 
only the error path which is immediately next to kobject_init_and_add() 
has a kobject_put() not any other error paths after it. Fix this by 
moving the kobject_put() into a goto label "err_other_attr_init:" and 
use that for error paths after kobject_init_and_add().

>> Both of these issues will be fixed when we add kobject_put() in the goto
>> label, as kfree() is already part of kobject_put().
> 
> No, you're fixing a problem in the patch which is not covered by moving
> kobject_put()!
Sure, I will try to rephrase it to:

1. Add a new label "unlock_drv_mutex"
2. Add a kfree() in the default statement of switch before going to 
"unlock_drv_mutex" to free up the memory allocated with kzalloc.
2. Move kobject_put() to goto "err_other_attr_init:" and use this goto 
label in every error path after kobject_init_and_add().

Please let me know if you have any comments.

Thank you very much.

Regards,
Harshit
>