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: <a0b5d36a-aad8-eaf5-7241-85d1c874ff8@linux.intel.com>
Date:   Mon, 13 Nov 2023 16:15:50 +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 Mon, 13 Nov 2023, Harshit Mogalapalli wrote:
> On 13/11/23 7:01 pm, Ilpo Järvinen wrote:
> > 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.
> > 
> Thanks for the suggestions.
> 
> > 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.
> > 
> 
> This is the problem I am trying to explain:
> 
> Let us say we do memory leak fixing in first patch:
> 
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> index 351d782f3e96..7f29a746210e 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/bioscfg.c
> @@ -612,6 +612,7 @@ static int hp_add_other_attributes(int attr_type)
>         default:
>                 pr_err("Error: Unknown attr_type: %d\n", attr_type);
>                 ret = -EINVAL;
> +               kfree(attr_name_kobj);
>                 goto err_other_attr_init;
>         }
> 
> @@ -637,8 +638,10 @@ static int hp_add_other_attributes(int attr_type)
>                 ret = -EINVAL;
>         }
> 
> -       if (ret)
> +       if (ret) {
> +               kfree(attr_name_kobj); ///// [1]

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

Solve each in own patch and don't mix the solutions.

I know both patches are needed to solve _both_ problems so it's fine to 
have "still broken" intermediate state as long as you didn't make things 
worse in the first patch which you didn't.

>                 goto err_other_attr_init;
> +       }
> 
>         mutex_unlock(&bioscfg_drv.mutex);
>         return 0;
> 
> Now when we want to move kobject_put() to goto label in next patch,
> we should remove the kfree [1], as kobject_put() already does a kfree().
> 
> If that sounds okay, I will split this into two patches, (first one, only
> fixing memory leak; and second one fixing missing kobject_put() calls on error
> paths)

-- 
 i.