Subject: Re: CVS commit: src/sys
To: Jachym Holecek <freza@NetBSD.org>
From: Bill Stouder-Studenmund <firstname.lastname@example.org>
Date: 06/24/2007 11:30:10
Content-Type: text/plain; charset=us-ascii
On Sat, Jun 23, 2007 at 08:11:18PM +0300, Elad Efrat wrote:
> Jachym Holecek wrote:
> >[Stripping CC somewhat]
> adding cc back. this is very relevant to all lists, and potentially
> current-users@ and netbsd-users@ as well, as this damages a framework
> in -current and future releases if it stays in the tree.
I expect the interface will get fixed quite soon.
> ># Elad Efrat 2007-06-23:
> >>while the changes to get/setgroups syscall internals and compat calls
> >>will not change the user experience in any way, breaking kauth's opacity
> >>have direct and immediate implications in the form of not allowing much
> >>flexibility when implementing new security models that expand beyond
> >>what is currently allowed by bsd44.
> >Could you provide some specific examples of what was possible before
> >but will be impossible because of David's change?
> >>additionally, it is well worth pointing out that the benefit you
> >>introduced is orthogonal to breaking the interface's opacity, and could
> >>have been introduced either way.
> >I don't quite see how opacity gets harmed -- the group list was a flat
> >array before and it's still a flat array now...
Because it now has to stay a flat array of gid_t's.
One whole point of kauth is that all the types are opaque. Subject to=20
change w/o notice. Something we can do WHATEVER we want with.
Like say start using GUIDs to identify groups.
Sure, doing that would mean a lot of compat code to map back & forth
between gid_t's. And these routines would be in the thick of it. But kauth
abstracts things well enough that we can do it.
ACLs are a place where this matters. Many file systems use GUIDs or SIDs=20
on-disk, so to add proper support, it'd be easiest if we used GUIDs (or=20
SIDs, but Apple went w/ GUIDs, so there's prior art) natively. Doing so=20
has a number of advantages, but that'd be getting farther off topic. :-)
> you're now getting a pointer to an internal buffer where you can change
> it directly without going through the interface.
Ok, so how about some techical discussion, eh?
1) We pass out the pointer to the group array. If we stored groups in some=
way other than gid_t's (say GUIDs), we'd have to malloc memory. We have no=
way to say, "We're done now," and so we would leak memory. Since we don't=
want to do a copy if we're about to do another copy, I can see wanting to=
not copy always. So we can't also just say, "call free()". So I think we=20
A) kauth_cred_getgrlist_done(kauth_cred_t cred, gid_t *) which will call=20
free if needed.
B) have a different routine that does the copy itself. Pass in a pointer,=
a vmspace identifier, and a count, and it handles getting the gid's there.=
It can either scribble to a kernel memory block (for compat code that has=
to do mapping) or copyout to userland directly.
2) We pass out a ointer to the group array. So right now, rogue code could=
scribble over the group list. I'm don't really think this is an issue as=20
any such rogue code would have to be in the kernel, and could probably=20
already scribble there if it wanted to.
3) We pass out the memory array, let the caller scribble on it, then=20
process it. This seems like a bad idea. I think what we should do is pass=
in an array and have kauth set the group list to that. I think changing=20
the group set should be atomic. Either it happens or it doesn't. Other=20
users (other threads in the same process for instance) should either see=20
the old group set or the new group set. Nothing else.
The current code, however, isn't atomic. We set the number of groups in
the kauth_cred_setngroups() call. We then let the caller do things. We=20
then kauth_authorize_process() the operation, and if it fails, we error=20
What's really unclear to me is how we are supposed to go back to the=20
original group set at this point. The caller has directly modified the=20
group set, and I see no way to restore the old contents.
4) Since we change the # groups before we finish changing the groups, we=20
can expose concurrent access to group IDs that aren't set yet. Say we=20
raise the count. Before we finish updating the array, other threads can=20
see garbage gids. That's bad.
5) There's nothing that protects against concurrent update. Yes, we have=20
calls to proc_crmod_enter() and proc_cr_mod_leav(), but the caller is=20
scriubbling over the group list when we don't have them held. Think about=
the mess that happens if two threads call into the set routine at once.=20
They both are scribbling over the group array at the same time.
6) In kauth_proc_setgroups(), there are two calls to proc_crmod_leave()=20
that have "ncred" and "cred" passed as different arguements. Is that=20
Earlier in the thread, it was mentioned that we get better performance if
we skip an unneeded malloc()/free() in the system callroutines. I agree. =
However we introduce a number of isses. So.... How often do we call these
routines? I thought a process calls setgroups() only once, and getgroups() =
a hand full of times over its lifetime. I don't think that a malloc()/
free() would really hurt us. And if it does, we can revisit. :-)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (NetBSD)
-----END PGP SIGNATURE-----