NetBSD Problem Report #44207

From Wolfgang.Stukenbrock@nagler-company.com  Wed Dec  8 15:11:30 2010
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 899EF63B87A
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  8 Dec 2010 15:11:30 +0000 (UTC)
Message-Id: <20101208151121.16C341E80CE@test-s0.nagler-company.com>
Date: Wed,  8 Dec 2010 16:11:21 +0100 (CET)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
X-Send-Pr-Version: 3.95

>Number:         44207
>Category:       kern
>Synopsis:       memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Dec 08 15:15:00 +0000 2010
>Closed-Date:    Thu Dec 09 09:11:59 +0000 2010
>Last-Modified:  Thu Dec 09 09:11:59 +0000 2010
>Originator:     Dr. W. Stukenbrock
>Release:        NetBSD 5.1
>Organization:
Dr. Nagler & Company GmbH
>Environment:


System: NetBSD test-s0 4.0 NetBSD 4.0 (NSW-WS) #0: Tue Aug 17 17:28:09 CEST 2010 wgstuken@test-s0:/usr/src/sys/arch/amd64/compile/NSW-WS amd64
Architecture: x86_64
Machine: amd64
>Description:
	While adding support for parity-maps handling the ioctl code for RAIDFRAME_GET_COMPONENT_LABEL
	has been changed.
	Accedently the memory allocated for the copyin is neither checked for an allocation error
	anymore, nor the memory is freed on copyin() error or bad values in the just copied in parameter.

	Another problem during attach of the raidframe driver is, that the number of available
	raid devices is not reset to 0 if no memory for the softc structures can be allocated.
	This of cause will be a very rare situation, but if it happens access to not-allocated
	memory may happen. (Found by checking all RF_Malloc()'s in this file ...)
>How-To-Repeat:
	Found by a look into the sources.
	You may trigger it by passing bad values in the parameter for the component-label-column.
>Fix:
	The following fix will remove both problems.

--- rf_netbsdkintf.c	2010/12/08 14:51:01	1.4
+++ rf_netbsdkintf.c	2010/12/08 15:03:48
@@ -387,6 +387,7 @@
 		       M_RAIDFRAME, M_NOWAIT);
 	if (raid_softc == NULL) {
 		aprint_error("WARNING: no memory for RAIDframe driver\n");
+		num_raid = 0; /* reset number of raid devices to 0 - no memory in our strucutres !!! */
 		return;
 	}

@@ -1226,11 +1227,14 @@
 		 * copy and hit the disk, as with disklabel(8).
 		 */
 		RF_Malloc(clabel, sizeof(*clabel), (RF_ComponentLabel_t *));
+		if (clabel == NULL)
+			return (ENOMEM);

 		retcode = copyin( *clabel_ptr, clabel,
 				  sizeof(RF_ComponentLabel_t));

 		if (retcode) {
+			RF_Free(clabel, sizeof(*clabel));
 			return(retcode);
 		}

@@ -1240,6 +1244,7 @@

 		if ((column < 0) || (column >= raidPtr->numCol +
 				     raidPtr->numSpare)) {
+			RF_Free(clabel, sizeof(*clabel));
 			return(EINVAL);
 		}


>Release-Note:

>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
Date: Wed, 8 Dec 2010 11:10:47 -0500

 On Dec 8,  3:15pm, Wolfgang.Stukenbrock@nagler-company.com (Wolfgang.Stukenbrock@nagler-company.com) wrote:
 -- Subject: kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LAB

 | 	While adding support for parity-maps handling the ioctl code for RAIDFRAME_GET_COMPONENT_LABEL
 | 	has been changed.
 | 	Accedently the memory allocated for the copyin is neither checked for an allocation error
 | 	anymore, nor the memory is freed on copyin() error or bad values in the just copied in parameter.

 There cannot be a memory allocation error because RF_Malloc does always WAITOK
 allocations.
 | 
 | 	Another problem during attach of the raidframe driver is, that the number of available
 | 	raid devices is not reset to 0 if no memory for the softc structures can be allocated.
 | 	This of cause will be a very rare situation, but if it happens access to not-allocated
 | 	memory may happen. (Found by checking all RF_Malloc()'s in this file ...)
 | >How-To-Repeat:
 | 	Found by a look into the sources.
 | 	You may trigger it by passing bad values in the parameter for the component-label-column.
 | >Fix:
 | 	The following fix will remove both problems.

 Thanks.

 christos

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44207 CVS commit: src/sys/dev/raidframe
Date: Wed, 8 Dec 2010 11:18:06 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Wed Dec  8 16:18:06 UTC 2010

 Modified Files:
 	src/sys/dev/raidframe: rf_netbsdkintf.c

 Log Message:
 PR/44207: Wolfgang.Stukenbrock:
     memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LAB


 To generate a diff of this commit:
 cvs rdiff -u -r1.276 -r1.277 src/sys/dev/raidframe/rf_netbsdkintf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Wed, 08 Dec 2010 16:27:19 +0000
State-Changed-Why:
christos committed, ok to close?


From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org,
        Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
Date: Wed, 08 Dec 2010 18:37:16 +0100

 Hi,

 if RF_Malloc() may never fail, there are lots of useless checks for NULL 
 of the allocated memory in this file ...
 Perhaps you should have a look at them too.

 W. Stukenbrock

 Christos Zoulas wrote:

 > The following reply was made to PR kern/44207; it has been noted by GNATS.
 > 
 > From: christos@zoulas.com (Christos Zoulas)
 > To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
 > 	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
 > Cc: 
 > Subject: Re: kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
 > Date: Wed, 8 Dec 2010 11:10:47 -0500
 > 
 >  On Dec 8,  3:15pm, Wolfgang.Stukenbrock@nagler-company.com (Wolfgang.Stukenbrock@nagler-company.com) wrote:
 >  -- Subject: kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LAB
 >  
 >  | 	While adding support for parity-maps handling the ioctl code for RAIDFRAME_GET_COMPONENT_LABEL
 >  | 	has been changed.
 >  | 	Accedently the memory allocated for the copyin is neither checked for an allocation error
 >  | 	anymore, nor the memory is freed on copyin() error or bad values in the just copied in parameter.
 >  
 >  There cannot be a memory allocation error because RF_Malloc does always WAITOK
 >  allocations.
 >  | 
 >  | 	Another problem during attach of the raidframe driver is, that the number of available
 >  | 	raid devices is not reset to 0 if no memory for the softc structures can be allocated.
 >  | 	This of cause will be a very rare situation, but if it happens access to not-allocated
 >  | 	memory may happen. (Found by checking all RF_Malloc()'s in this file ...)
 >  | >How-To-Repeat:
 >  | 	Found by a look into the sources.
 >  | 	You may trigger it by passing bad values in the parameter for the component-label-column.
 >  | >Fix:
 >  | 	The following fix will remove both problems.
 >  
 >  Thanks.
 >  
 >  christos
 >  
 > 


 -- 


 Dr. Nagler & Company GmbH
 Hauptstraße 9
 92253 Schnaittenbach

 Tel. +49 9622/71 97-42
 Fax +49 9622/71 97-50

 Wolfgang.Stukenbrock@nagler-company.com
 http://www.nagler-company.com


 Hauptsitz: Schnaittenbach
 Handelregister: Amberg HRB
 Gerichtsstand: Amberg
 Steuernummer: 201/118/51825
 USt.-ID-Nummer: DE 273143997
 Geschäftsführer: Dr. Martin Nagler, Dr. Dr. Karl-Kuno Kunze


From: Greg Oster <oster@cs.usask.ca>
To: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@NetBSD.org,
 gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org
Subject: Re: kern/44207: memory-leak in
 raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
Date: Wed, 8 Dec 2010 12:02:17 -0600

 On Wed, 08 Dec 2010 18:37:16 +0100
 Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com> wrote:

 > Hi,
 >=20
 > if RF_Malloc() may never fail, there are lots of useless checks for
 > NULL of the allocated memory in this file ...
 > Perhaps you should have a look at them too.

 heh.. I wonder why I never noticed that years ago when I did the
 big memory allocation changes... There's lots of those checks that can
 get ripped out.... (and some of them propagate to further useless
 checks.. :-/ ) =20

 Thanks for finding these...

 Later...

 Greg Oster

 > Christos Zoulas wrote:
 >=20
 > > The following reply was made to PR kern/44207; it has been noted by
 > > GNATS.
 > >=20
 > > From: christos@zoulas.com (Christos Zoulas)
 > > To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org,=20
 > > 	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
 > > Cc:=20
 > > Subject: Re: kern/44207: memory-leak in
 > > raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL) Date: Wed, 8 Dec 2010
 > > 11:10:47 -0500
 > >=20
 > >  On Dec 8,  3:15pm, Wolfgang.Stukenbrock@nagler-company.com
 > > (Wolfgang.Stukenbrock@nagler-company.com) wrote: -- Subject:
 > > kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LAB=20
 > >  | 	While adding support for parity-maps handling the ioctl
 > > code for RAIDFRAME_GET_COMPONENT_LABEL | 	has been changed.
 > >  | 	Accedently the memory allocated for the copyin is
 > > neither checked for an allocation error | 	anymore, nor the
 > > memory is freed on copyin() error or bad values in the just copied
 > > in parameter. There cannot be a memory allocation error because
 > > RF_Malloc does always WAITOK allocations.
 > >  |=20
 > >  | 	Another problem during attach of the raidframe driver
 > > is, that the number of available | 	raid devices is not
 > > reset to 0 if no memory for the softc structures can be allocated.
 > > | 	This of cause will be a very rare situation, but if it
 > > happens access to not-allocated | 	memory may happen. (Found
 > > by checking all RF_Malloc()'s in this file ...) | >How-To-Repeat: |
 > > 	Found by a look into the sources. | 	You may trigger
 > > it by passing bad values in the parameter for the
 > > component-label-column. | >Fix: | 	The following fix will
 > > remove both problems.=20
 > >  Thanks.
 > > =20
 > >  christos
 > > =20
 > >=20
 >=20
 >=20
 > --=20
 >=20
 >=20
 > Dr. Nagler & Company GmbH
 > Hauptstra=DFe 9
 > 92253 Schnaittenbach
 >=20
 > Tel. +49 9622/71 97-42
 > Fax +49 9622/71 97-50
 >=20
 > Wolfgang.Stukenbrock@nagler-company.com
 > http://www.nagler-company.com
 >=20
 >=20
 > Hauptsitz: Schnaittenbach
 > Handelregister: Amberg HRB
 > Gerichtsstand: Amberg
 > Steuernummer: 201/118/51825
 > USt.-ID-Nummer: DE 273143997
 > Gesch=E4ftsf=FChrer: Dr. Martin Nagler, Dr. Dr. Karl-Kuno Kunze
 >=20


 Later...

 Greg Oster

From: christos@zoulas.com (Christos Zoulas)
To: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>, 
	gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org, 
	netbsd-bugs@NetBSD.org
Subject: Re: kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
Date: Wed, 8 Dec 2010 13:56:54 -0500

 On Dec 8,  6:37pm, Wolfgang.Stukenbrock@nagler-company.com (Wolfgang Stukenbrock) wrote:
 -- Subject: Re: kern/44207: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT

 | Hi,
 | 
 | if RF_Malloc() may never fail, there are lots of useless checks for NULL 
 | of the allocated memory in this file ...
 | Perhaps you should have a look at them too.
 | 

 The raidframe code needs a lot of cleanup... I guess I'll fix RF_Malloc
 at some point.

 christos

From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, netbsd-bugs@NetBSD.org, gnats-admin@NetBSD.org,
        wiz@NetBSD.org, Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: kern/44207 (memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL))
Date: Thu, 09 Dec 2010 09:39:53 +0100

 Hi,

 now the diff is visible in CVS-web ...

 The patch looks good, you may close the PR.

 Best regards

 W. Stukenbrock

 wiz@NetBSD.org wrote:

 > Synopsis: memory-leak in raid-ioctl(RAIDFRAME_GET_COMPONENT_LABEL)
 > 
 > State-Changed-From-To: open->feedback
 > State-Changed-By: wiz@NetBSD.org
 > State-Changed-When: Wed, 08 Dec 2010 16:27:19 +0000
 > State-Changed-Why:
 > christos committed, ok to close?
 > 
 > 
 > 
 > 


 -- 


 Dr. Nagler & Company GmbH
 Hauptstraße 9
 92253 Schnaittenbach

 Tel. +49 9622/71 97-42
 Fax +49 9622/71 97-50

 Wolfgang.Stukenbrock@nagler-company.com
 http://www.nagler-company.com


 Hauptsitz: Schnaittenbach
 Handelregister: Amberg HRB
 Gerichtsstand: Amberg
 Steuernummer: 201/118/51825
 USt.-ID-Nummer: DE 273143997
 Geschäftsführer: Dr. Martin Nagler, Dr. Dr. Karl-Kuno Kunze


State-Changed-From-To: feedback->closed
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Thu, 09 Dec 2010 09:11:59 +0000
State-Changed-Why:
Confirmed fixed, thanks.


>Unformatted:

Home	PR Database Search