linux-kernel - Re: [PATCH][QUESTION] Intentional memory leak in ipmi

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: <20160222191946.GA980385@devbig337.prn1.facebook.com>
Date:	Mon, 22 Feb 2016 11:19:46 -0800
From:	Calvin Owens <calvinowens@...com>
To:	Corey Minyard <minyard@....org>
CC:	<openipmi-developer@...ts.sourceforge.net>,
	<linux-kernel@...r.kernel.org>, <kernel-team@...com>
Subject: Re: [PATCH][QUESTION] Intentional memory leak in ipmi_msghandler?

On Friday 02/19 at 07:14 -0600, Corey Minyard wrote:
> On 02/19/2016 12:41 AM, Calvin Owens wrote:
> >Hello,
> >
> >I've got a few boxes that are leaking memory in handle_new_recv_msgs()
> >in ipmi_msghandler. AFAICS this is intentional, there's even an explicit
> >counter that tracks the number of times smi_msg is leaked.
> 
> Are you 100% sure about this? 

I'm absolutely certain this is where I'm leaking: I threw in a printk()
and saw exact correlation between the number of times I saw that and the
number of kmalloc-1024 objects leaked. But...

> There's no intentional leak, a negative return from this function
> means the message was used for another purpose and thus shouldn't be
> freed.  There's only one situation where this happens and you should
> never hit it in normal operation.

This is actually extremely helpful: we have some horrible non-upstream code
behind this that I thought I had ruled out being at fault here, but this
sounds like it might be. In any case, I won't waste your time any more
until I can reproduce it on upstream (which is unfortunately impossible
on this particular HW I see the leak on, otherwise I would have done that
in the first place...).

Thanks very much for the prompt response, I really appreciate it.

Calvin

> >I'm guessing there was a reason for doing this, but there wasn't any
> >discussion about it on LKML when the patch was accepted. Can you clarify
> >why something like the below patch won't work? I tried it on one of my
> >leaky boxes and nothing obviously horrible happened.
> 
> Well, that's because nothing probably happened, and it probably had no
> effect on the leak.  A better comment on this code is probably in
> order.  But that patch is incorrect.
> 
> I doubt the leak is here.  If you are having a leak, is it possible to
> characterize it better?  Are you handling commands from IPMB?  Are you
> handling LAN commands here?
> 
> -corey
> 
> >
> >Thanks,
> >Calvin
> >
> >----8<----
> >From: Calvin Owens <calvinowens@...com>
> >Subject: [PATCH] ipmi_msghandler: Don't leak memory on errors
> >
> >Signed-off-by: Calvin Owens <calvinowens@...com>
> >---
> >  drivers/char/ipmi/ipmi_msghandler.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> >
> >diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
> >index 94fb407..ed82ffa 100644
> >--- a/drivers/char/ipmi/ipmi_msghandler.c
> >+++ b/drivers/char/ipmi/ipmi_msghandler.c
> >@@ -3834,10 +3834,7 @@ static void handle_new_recv_msgs(ipmi_smi_t intf)
> >  			break;
> >  		} else {
> >  			list_del(&smi_msg->link);
> >-			if (rv == 0)
> >-				/* Message handled */
> >-				ipmi_free_smi_msg(smi_msg);
> >-			/* If rv < 0, fatal error, del but don't free. */
> >+			ipmi_free_smi_msg(smi_msg);
> >  		}
> >  	}
> >  	if (!run_to_completion)
>