linux-kernel - Re: [PATCH 3/3] ipmi: fix memleak when unload ipmi driver

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: <Y0CDRS+ppvKTiGml@minyard.net>
Date:   Fri, 7 Oct 2022 14:51:33 -0500
From:   Corey Minyard <minyard@....org>
To:     Zhang Yuchen <zhangyuchen.lcr@...edance.com>
Cc:     openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, qi.zheng@...ux.dev
Subject: Re: [PATCH 3/3] ipmi: fix memleak when unload ipmi driver

On Fri, Oct 07, 2022 at 05:26:17PM +0800, Zhang Yuchen wrote:
> After the IPMI disconnect problem, the memory kept rising and we tried
> to unload the driver to free the memory. However, only part of the
> free memory is recovered after the driver is uninstalled. Using
> ebpf to hook free functions, we find that neither ipmi_user nor
> ipmi_smi_msg is free, only ipmi_recv_msg is free.
> 
> We find that the deliver_smi_err_response call in clean_smi_msgs does
> the destroy processing on each message from the xmit_msg queue without
> checking the return value and free ipmi_smi_msg.
> 
> deliver_smi_err_response is called only at this location. Adding the
> free handling has no effect.
> 
> To verify, try using ebpf to trace the free function.
> 
>   $ bpftrace -e 'kretprobe:ipmi_alloc_recv_msg {printf("alloc rcv
>       %p\n",retval);} kprobe:free_recv_msg {printf("free recv %p\n",
>       arg0)} kretprobe:ipmi_alloc_smi_msg {printf("alloc smi %p\n",
>         retval);} kprobe:free_smi_msg {printf("free smi  %p\n",arg0)}'
> 
> Signed-off-by: Zhang Yuchen <zhangyuchen.lcr@...edance.com>
> ---
>  drivers/char/ipmi/ipmi_msghandler.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> index c8a3b208f923..7a7534046b5b 100644
> --- a/drivers/char/ipmi/ipmi_msghandler.c
> +++ b/drivers/char/ipmi/ipmi_msghandler.c
> @@ -3710,12 +3710,15 @@ static void deliver_smi_err_response(struct ipmi_smi *intf,
>  				     struct ipmi_smi_msg *msg,
>  				     unsigned char err)
>  {
> +	int rv;
>  	msg->rsp[0] = msg->data[0] | 4;
>  	msg->rsp[1] = msg->data[1];
>  	msg->rsp[2] = err;
>  	msg->rsp_size = 3;
>  	/* It's an error, so it will never requeue, no need to check return. */

The above comment is wrong, but yes, this is correct.  I'll queue this
and remove the comment.

Thanks,

-corey

> -	handle_one_recv_msg(intf, msg);
> +	rv = handle_one_recv_msg(intf, msg);
> +	if (rv == 0)
> +		ipmi_free_smi_msg(msg);
>  }
>  
>  static void cleanup_smi_msgs(struct ipmi_smi *intf)
> -- 
> 2.30.2
>