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

Message-ID: <24d6b879-6874-22f7-c8e6-90f3f0968d9f@linux.intel.com>
Date:   Mon, 13 Nov 2023 18:44:28 +0200 (EET)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Dan Carpenter <dan.carpenter@...aro.org>
cc:     Harshit Mogalapalli <harshit.m.mogalapalli@...cle.com>,
        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>,
        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 Mon, 13 Nov 2023, Dan Carpenter wrote:

> On Mon, Nov 13, 2023 at 04:15:50PM +0200, Ilpo Järvinen wrote:
> > This relates to the 2nd problem (missing kobject_put()) and will be 
> > covered by the other patch. Don't try to solve this in the first patch
> > at all!
> > 
> > There are two indepedent problems:
> > - Before kobject_init_and_add(), kfree() is missing
> > - After kobject_init_and_add(), kobject_put() is missing
> 
> It's the same problem, though. The attr_name_kobj is leaked on all the
> error paths. 

I'll have politely disagree beyond that the symptoms are indeed about the 
same, the problem is clearly different like you immediately admit even 
yourself by stating this: ;-)

> It's just that it needs to be freed different ways depending on where 
> you are.

...And that's because "it" actually changed in between so the problem 
became a different one.

> To me splitting it up makes it harder to review

This has already been proven incorrect in the context of this patch so 
your argument is rather weak... While reviewing it I clearly noted that 
the different way of handling things was not properly covered, and that 
was because what needs to be "freed" was changed by 
kobject_init_and_add(). If one would have done them separately, each 
commit message would have been more to the point and it would have been 
simpler to review which is exactly the opposite to your claim. But I guess 
we'll end up disagreing on this too :-).

> and I would not allow it in Staging. You can't fix half the problem.

I don't have that strong opinion on this so Harshit please follow what 
Dan is suggesting, just fix the changelog to clearly cover both cases.

-- 
 i.