Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753096AbYAMAkH (ORCPT ); Sat, 12 Jan 2008 19:40:07 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753928AbYAMAjw (ORCPT ); Sat, 12 Jan 2008 19:39:52 -0500 Received: from ug-out-1314.google.com ([66.249.92.171]:33878 "EHLO ug-out-1314.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753927AbYAMAjv (ORCPT ); Sat, 12 Jan 2008 19:39:51 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:to:cc:subject:message-id:user-agent:mime-version:content-type:from; b=xLh/8TojWAiI3vlVfE0kyeO4vWIG0VapTaQKWGytwrU6L8Va6K52/YzPAA5w8arxHjBkItILlT9aeMOYs/43mu1ZwCaKv3jOp6IqxnjAfcF98hDID4efpIN13ro6GBfO2YKMpd/RdEprvHxjSlpmhCaI/ufxie+/v9hBBpsxdPA= Date: Sun, 13 Jan 2008 01:34:44 +0100 (CET) To: Samuel Ortiz cc: Dag Brattli , Jean Tourrilhes , LKML , Netdev , Jesper Juhl Subject: [PATCH 2/2] irda: avoid potential memory leak in irda_setsockopt() Message-ID: User-Agent: Alpine 1.00 (LNX 882 2007-12-20) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII From: Jesper Juhl Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3892 Lines: 117 There are paths through the irda_setsockopt() function where we return and may or may not have allocated a new ias_obj but in any case have not used it for anything yet and we end up leaking memory. As far as I can tell, in the case where we didn't allocate a new ias_ob but simply were planning to use one already available then we should not free it before returning. But when we have allocated a brand new ias_obj but have not yet used it or put it on any lists etc and then return, that's a memory leak. There are two cases: 1) switch (optname) { case IRLMP_IAS_SET: ... if(ias_obj == (struct ias_object *) NULL) { /* Create a new object */ --[alloc]--> ias_obj = irias_new_object(ias_opt->irda_class_name, jiffies); ... switch(ias_opt->irda_attrib_type) { case IAS_OCT_SEQ: /* Check length */ if(ias_opt->attribute.irda_attrib_octet_seq.len > IAS_MAX_OCTET_STRING) { kfree(ias_opt); --[leak]--> return -EINVAL; ... } irias_insert_object(ias_obj); The allocated object isn't referenced at all until we get outside the inner switch, so clearly we leak it (if we took the path that allocated it that is). 2) The second case is the same as the above, except it's the default: case in the inner switch instead of case IAS_OCT_SEQ: default : kfree(ias_opt); return -EINVAL; The way I propose to fix this is with a new variable that keeps track of whether or not we found an existing ias_obj to use or if we took the allocation path, then use that variable to determine if we should free ias_obj before returning from the function. I'm not very intimate with this code, so if there's a better solution I'd very much like to hear it. It's also entirely possible that someone with more knowledge of this code can prove that these cases can't actually ever happen - if that's the case then please let me know. This patch is meant to be applied on top of [PATCH 1/2] irda: return -ENOMEM upon failure to allocate new ias_obj The Coverity checker gets credit for pointing its finger towards this. Signed-off-by: Jesper Juhl --- af_irda.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c index e33f0a5..352e8a7 100644 --- a/net/irda/af_irda.c +++ b/net/irda/af_irda.c @@ -1824,7 +1824,8 @@ static int irda_setsockopt(struct socket *sock, int level, int optname, struct irda_sock *self = irda_sk(sk); struct irda_ias_set *ias_opt; struct ias_object *ias_obj; - struct ias_attrib * ias_attr; /* Attribute in IAS object */ + struct ias_attrib *ias_attr; /* Attribute in IAS object */ + int alloc_new_obj = 0; int opt; IRDA_DEBUG(2, "%s(%p)\n", __FUNCTION__, self); @@ -1885,6 +1886,7 @@ static int irda_setsockopt(struct socket *sock, int level, int optname, kfree(ias_opt); return -ENOMEM; } + alloc_new_obj = 1; } /* Do we have the attribute already ? */ @@ -1908,6 +1910,8 @@ static int irda_setsockopt(struct socket *sock, int level, int optname, if(ias_opt->attribute.irda_attrib_octet_seq.len > IAS_MAX_OCTET_STRING) { kfree(ias_opt); + if (alloc_new_obj) + kfree(ias_obj); return -EINVAL; } /* Add an octet sequence attribute */ @@ -1936,6 +1940,8 @@ static int irda_setsockopt(struct socket *sock, int level, int optname, break; default : kfree(ias_opt); + if (alloc_new_obj) + kfree(ias_obj); return -EINVAL; } irias_insert_object(ias_obj); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/