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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <1b58df2d-b444-ddb7-7533-9911d35f8f7@linux.intel.com>
Date:   Mon, 13 Nov 2023 15:31:48 +0200 (EET)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Harshit Mogalapalli <harshit.m.mogalapalli@...cle.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()

On Sat, 11 Nov 2023, Harshit Mogalapalli wrote:
> 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.

I meant that in the first patch you fix add the missing kfree(). Then in 
the second one, you correct kobject_put() + play with goto labels. There's 
no extra code that needs to be added and then removed AFAICT.

That way, you can make the commit messages more to the point too per 
patch.

> > > 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().

This is easier to understand I think:

kobject_put() must be always called after passing the object to 
kobject_init_and_add(). Only the error path which is immediately next
to kobject_init_and_add() calls kobject_put() and not any other error 
path after it.

Fix the error handling by moving the kobject_put() into the goto label 
err_other_attr_init that is already used by all the 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.

I think none of this is needed for this patch after you move the other fix 
into a separate patch. Those two paragraphs I suggest above would explain 
the problem and solution (no need to have these numbered bullets or 
anything else besides those 2 paragraphs).

-- 
 i.