Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Corey Minyard <tcminyard@gmail.com>
Date:   Thu, 29 Jun 2023 08:07:34 -0500
From:   Corey Minyard <minyard@acm.org>
To:     "GONG, Ruiqi" <gongruiqi@huaweicloud.com>
Cc:     Yi Yang <yiyang13@huawei.com>,
        openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Wang Weiyang <wangweiyang2@huawei.com>,
        Xiu Jianfeng <xiujianfeng@huawei.com>, gongruiqi1@huawei.com
Subject: Re: [PATCH] ipmi_si: fix a memleak in try_smi_init()
Message-ID: <ZJ2CFthVKdr0U29K@mail.minyard.net>
Reply-To: minyard@acm.org
References: <20230629123328.2402075-1-gongruiqi@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230629123328.2402075-1-gongruiqi@huaweicloud.com>
Precedence: bulk

On Thu, Jun 29, 2023 at 08:33:28PM +0800, GONG, Ruiqi wrote:
> From: Yi Yang <yiyang13@huawei.com>
> 
> Kmemleak reported the following leak info in try_smi_init():
> 
> unreferenced object 0xffff00018ecf9400 (size 1024):
>   comm "modprobe", pid 2707763, jiffies 4300851415 (age 773.308s)
>   backtrace:
>     [<000000004ca5b312>] __kmalloc+0x4b8/0x7b0
>     [<00000000953b1072>] try_smi_init+0x148/0x5dc [ipmi_si]
>     [<000000006460d325>] 0xffff800081b10148
>     [<0000000039206ea5>] do_one_initcall+0x64/0x2a4
>     [<00000000601399ce>] do_init_module+0x50/0x300
>     [<000000003c12ba3c>] load_module+0x7a8/0x9e0
>     [<00000000c246fffe>] __se_sys_init_module+0x104/0x180
>     [<00000000eea99093>] __arm64_sys_init_module+0x24/0x30
>     [<0000000021b1ef87>] el0_svc_common.constprop.0+0x94/0x250
>     [<0000000070f4f8b7>] do_el0_svc+0x48/0xe0
>     [<000000005a05337f>] el0_svc+0x24/0x3c
>     [<000000005eb248d6>] el0_sync_handler+0x160/0x164
>     [<0000000030a59039>] el0_sync+0x160/0x180
> 
> The problem was that when an error occurred before handlers registration
> and after allocating `new_smi->si_sm`, the variable wouldn't be freed in
> the error handling afterwards since `shutdown_smi()` hadn't been
> registered yet. Fix it by adding a `kfree()` in the error handling path
> in `try_smi_init()`.

Thanks, I have included this.  And thanks for handling the stable thing,
too.

-corey

> 
> Cc: stable@vger.kernel.org # 4.19+
> Fixes: 7960f18a5647 ("ipmi_si: Convert over to a shutdown handler")
> Signed-off-by: Yi Yang <yiyang13@huawei.com>
> Co-developed-by: GONG, Ruiqi <gongruiqi@huaweicloud.com>
> Signed-off-by: GONG, Ruiqi <gongruiqi@huaweicloud.com>
> ---
>  drivers/char/ipmi/ipmi_si_intf.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index abddd7e43a9a..5cd031f3fc97 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -2082,6 +2082,11 @@ static int try_smi_init(struct smi_info *new_smi)
>  		new_smi->io.io_cleanup = NULL;
>  	}
>  
> +	if (rv && new_smi->si_sm) {
> +		kfree(new_smi->si_sm);
> +		new_smi->si_sm = NULL;
> +	}
> +
>  	return rv;
>  }
>  
> -- 
> 2.25.1
>