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.
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!
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; }
Yup, this one worked just fine. Hope this bug won't happen elsewhere :) I guess this report can be closed. -- Archer
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"
State Changed From-To: open->patched Committed patch to HEAD. Awaiting merge to 8.
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"
State Changed From-To: patched->closed Merged to 8