Bug 138657 - [panic] [patch] Kernel memory corruption, ucred.cr_groups[] [regression]
Summary: [panic] [patch] Kernel memory corruption, ucred.cr_groups[] [regression]
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 8.0-BETA4
Hardware: Any Any
: Normal Affects Only Me
Assignee: Brooks Davis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-09 10:10 UTC by GNATS administrator
Modified: 2009-09-24 22:46 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description GNATS administrator freebsd_committer freebsd_triage 2009-09-09 10:10:03 UTC
Lets take a look at this code in ufs_vnops.c file:
refcount_init(&ucred.cr_ref, 1);
ucred.cr_uid = ip->i_uid;
ucred.cr_ngroups = 1;
ucred.cr_groups[0] = dp->i_gid;<---problem here
ucp = &ucred;

ucred structure variable is used here. It's a local structure so it's allocated on stack. Now lets take a look at the structure definition:
struct ucred {
--skipped...
gid_t *cr_groups;
--skipped...
};

As we see we've got a pointer. But this pointer is not initialized at the point of usage. So it contains some arbitrary value from the stack. And then that memory gets overwritten. Lets take a look at the assembly listing:
mov edx, [esi+6Ch]
mov eax, [ebp+var_140]<---here we read some arbitrary pointer from the stack
mov [eax], edx<---and here we smash the data at our arbitrary pointer

In my case some memory around ufs_lookup was overwritten.

Fix: 

In FreeBSD 7.1 ucred structure was defined with cr_groups array instead or pointer. So at some places in 8.0 version this was fixed by creating local gid_t and assigning it's address to the pointer. And some places (and described above is one of them was missed for unknown reason). 
I suppose we should allocate memory for our pointer before using it so recklessly. It's also possible to create a local gid_t and assigning it's address to the pointer (this is done a bit lower at ufs_vnops.c file). And I suggest to review the code that works with this pointer, the same mistake may happen elsewhere.
How-To-Repeat: Enable QUOTA and SUIDDIR at the kernel options, set them on some partition and try to create a directory there.
For me worked tar -xvjf phpMyAdmin-3.2.1-all-languages.tar.bz2 which also calls mkdir when extracting files.
Comment 1 Gavin Atkinson freebsd_committer freebsd_triage 2009-09-09 13:30:06 UTC
Responsible Changed
From-To: freebsd-bugs->brooks

Brooks, could you take a look at this please?  It appears to be fallout 
from the ucred changes.  THanks!
Comment 2 Brooks Davis freebsd_committer freebsd_triage 2009-09-09 15:03:06 UTC
Thanks for your report and detailed analysis.  I'm not sure how I
handled the second instance of this in ufs_vnops.c and missed the first.
Here's a patch that I believe should work around this.  Can you test it?

I'd prefer we changed this code to have a credential with proper life
cycle management even if we just create one at boot and hang it on a
global variable, but adding extra storeage on the stack seemed less
disruptive for the first cut.

-- Brooks

Index: ufs_vnops.c
===================================================================
--- ufs_vnops.c	(revision 196736)
+++ ufs_vnops.c	(working copy)
@@ -1449,6 +1449,7 @@
 	{
 #ifdef QUOTA
 		struct ucred ucred, *ucp;
+		gid_t ucred_group;
 		ucp = cnp->cn_cred;
 #endif
 		/*
@@ -1476,6 +1477,7 @@
 				refcount_init(&ucred.cr_ref, 1);
 				ucred.cr_uid = ip->i_uid;
 				ucred.cr_ngroups = 1;
+				ucred.cr_groups = &ucred_group;
 				ucred.cr_groups[0] = dp->i_gid;
 				ucp = &ucred;
 			}
Comment 3 fghjfg hjfg 2009-09-09 16:38:10 UTC
Yup, this one worked just fine. Hope this bug won't happen elsewhere :) I guess this report can be closed.

-- Archer
Comment 4 dfilter service freebsd_committer freebsd_triage 2009-09-17 13:35:27 UTC
Author: brooks
Date: Thu Sep 17 12:35:13 2009
New Revision: 197269
URL: http://svn.freebsd.org/changeset/base/197269

Log:
  Allocate space for the group array in a static credential used in
  the quota code.  One case was correctly handled in r194498, but
  this one was missed.
  
  PR:		kern/138657
  Tested by:	PR submitter
  MFC after:	3 days

Modified:
  head/sys/ufs/ufs/ufs_vnops.c

Modified: head/sys/ufs/ufs/ufs_vnops.c
==============================================================================
--- head/sys/ufs/ufs/ufs_vnops.c	Thu Sep 17 12:27:06 2009	(r197268)
+++ head/sys/ufs/ufs/ufs_vnops.c	Thu Sep 17 12:35:13 2009	(r197269)
@@ -1449,6 +1449,7 @@ ufs_mkdir(ap)
 	{
 #ifdef QUOTA
 		struct ucred ucred, *ucp;
+		gid_t ucred_group;
 		ucp = cnp->cn_cred;
 #endif
 		/*
@@ -1476,6 +1477,7 @@ ufs_mkdir(ap)
 				refcount_init(&ucred.cr_ref, 1);
 				ucred.cr_uid = ip->i_uid;
 				ucred.cr_ngroups = 1;
+				ucred.cr_groups = &ucred_group;
 				ucred.cr_groups[0] = dp->i_gid;
 				ucp = &ucred;
 			}
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 5 Brooks Davis freebsd_committer freebsd_triage 2009-09-17 13:38:50 UTC
State Changed
From-To: open->patched

Committed patch to HEAD.  Awaiting merge to 8.
Comment 6 dfilter service freebsd_committer freebsd_triage 2009-09-24 22:33:07 UTC
Author: brooks
Date: Thu Sep 24 21:32:56 2009
New Revision: 197473
URL: http://svn.freebsd.org/changeset/base/197473

Log:
  MFC r197269:
  Allocate space for the group array in a static credential used in
  the quota code.  One case was correctly handled in r194498, but
  this one was missed.
  
  PR:		kern/138657
  Tested by:	PR submitter
  MFC after:	3 days
  Approved by:	re@ (kib)

Modified:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)
  stable/8/sys/dev/xen/xenpci/   (props changed)
  stable/8/sys/ufs/ufs/ufs_vnops.c

Modified: stable/8/sys/ufs/ufs/ufs_vnops.c
==============================================================================
--- stable/8/sys/ufs/ufs/ufs_vnops.c	Thu Sep 24 20:43:08 2009	(r197472)
+++ stable/8/sys/ufs/ufs/ufs_vnops.c	Thu Sep 24 21:32:56 2009	(r197473)
@@ -1449,6 +1449,7 @@ ufs_mkdir(ap)
 	{
 #ifdef QUOTA
 		struct ucred ucred, *ucp;
+		gid_t ucred_group;
 		ucp = cnp->cn_cred;
 #endif
 		/*
@@ -1476,6 +1477,7 @@ ufs_mkdir(ap)
 				refcount_init(&ucred.cr_ref, 1);
 				ucred.cr_uid = ip->i_uid;
 				ucred.cr_ngroups = 1;
+				ucred.cr_groups = &ucred_group;
 				ucred.cr_groups[0] = dp->i_gid;
 				ucp = &ucred;
 			}
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
Comment 7 Brooks Davis freebsd_committer freebsd_triage 2009-09-24 22:44:49 UTC
State Changed
From-To: patched->closed

Merged to 8