Some context for this change from IRC: we're in a small minority of systems today that continues to conflate the egid and supplemental groups as we do, which makes some uses of setgroups() a landmine if they're assuming FreeBSD behavior of also setting the egid. It would be good to start investigating heading in the direction of the rest of the world and separating cr_gid out to avoid this one weird difference with security implications for software ported to other systems.

After having written some inline comments, it now appears to me that we may not want to substitute cr_groups[0] with cr_gid for objects of type struct xucred in the kernel, but on the contrary ensure that references to the egid in struct xucred are always made through cr_groups[0]. Why is that, you may ask? Because of the fact that, currently, field names are shared between struct ucred and struct xucred, and I suspect we may want to switch struct ucred (internal) independently of struct xucred (exported, and used, e.g., for LOCAL_PEERCRED), most probably doing struct ucred before struct xucred. In this case, we will want to preserve the fact that cr_uid is defined to cr_groups[0] for userland, but obviously that should not apply in the kernel when egid is stored in a real cr_gid field in struct ucred, hence the need for an explicit cr_groups[0] inside the kernel. That said, it is possible (and probably a better idea) to convert references to cr_groups[0] on struct xucred into a function call taking a struct ucred (e.g., xucred_gid()) and that returns a pointer to the right slot.

I would generally replace the XXXKE comments by code that supports both the current state and the cr_gid being split state (see inline comments for suggestions on how to do this), but that could be done as a separate step.

A lot more places need changes at the moment of the split than those commented here. In particular (probably not exhaustive list):

• groups_is_supplementary()
• setcred() & co.
• Helpers such as groups_check_normalized(), groups_check_positive_len(), groups_normalize(), crsetgroups(), etc.

If keeping the XXXKE comments, please explicitly say in the commit message that they do not form an exhaustive list.

Depending on you plans, I could perhaps take on some of the burden of the actual split.

One note: We could actually just change the contract of setgroups() without actually splitting cr_gid on struct ucred, if that is pressing and we lack time for the full split. I really think we should get this into 15.0 rather than 16.0.

In D51153#1167511, @olce wrote:

After having written some inline comments, it now appears to me that we may not want to substitute cr_groups[0] with cr_gid for objects of type struct xucred in the kernel, but on the contrary ensure that references to the egid in struct xucred are always made through cr_groups[0]. Why is that, you may ask? Because of the fact that, currently, field names are shared between struct ucred and struct xucred, and I suspect we may want to switch struct ucred (internal) independently of struct xucred (exported, and used, e.g., for LOCAL_PEERCRED), most probably doing struct ucred before struct xucred. In this case, we will want to preserve the fact that cr_uid is defined to cr_groups[0] for userland, but obviously that should not apply in the kernel when egid is stored in a real cr_gid field in struct ucred, hence the need for an explicit cr_groups[0] inside the kernel. That said, it is possible (and probably a better idea) to convert references to cr_groups[0] on struct xucred into a function call taking a struct ucred (e.g., xucred_gid()) and that returns a pointer to the right slot.

My follow-up change that I'm toying with locally that actually splits cr_gid out solves this instead with some union trickery in struct xucred:

short   cr_ngroups;             /* number of groups (incl. cr_gid). */  
union {                                                                 
        /*                                                              
         * Special little hack to avoid needing a cr_gid macro, which   
         * would cause problems if one were to use it with struct ucred 
         * which also has a cr_groups member.                           
         */                                                             
        struct {                                                        
                gid_t   cr_gid;         /* effective group id */        
                gid_t   cr_sgroups[XU_NGROUPS - 1];                     
        };                                                              
                                                                        
        gid_t   cr_groups[XU_NGROUPS];  /* groups */                    
};

I would generally replace the XXXKE comments by code that supports both the current state and the cr_gid being split state (see inline comments for suggestions on how to do this), but that could be done as a separate step.

A lot more places need changes at the moment of the split than those commented here. In particular (probably not exhaustive list):

• groups_is_supplementary()
• setcred() & co.
• Helpers such as groups_check_normalized(), groups_check_positive_len(), groups_normalize(), crsetgroups(), etc.

If keeping the XXXKE comments, please explicitly say in the commit message that they do not form an exhaustive list.

Depending on you plans, I could perhaps take on some of the burden of the actual split.

Right, this diff was intended to do all of the mechanical conversions. You can see a prototype of the split that I've started preparing here: https://people.freebsd.org/~kevans/cr_gid.diff

This also tries to handle the xucred problem mentioned above by simply punting on it entirely; we get a cr_gid member that's aliased to the first member of cr_groups. It avoids changing setgroups()/getgroups() for the initial split, continuing to use the first member as the egid and writing out the egid as the first member for the time being so that we can audit userland usage separately.

In D51153#1167532, @kevans wrote:

My follow-up change that I'm toying with locally that actually splits cr_gid out solves this instead with some union trickery in struct xucred:

short   cr_ngroups;             /* number of groups (incl. cr_gid). */  
union {                                                                 
        /*                                                              
         * Special little hack to avoid needing a cr_gid macro, which   
         * would cause problems if one were to use it with struct ucred 
         * which also has a cr_groups member.                           
         */                                                             
        struct {                                                        
                gid_t   cr_gid;         /* effective group id */        
                gid_t   cr_sgroups[XU_NGROUPS - 1];                     
        };                                                              
                                                                        
        gid_t   cr_groups[XU_NGROUPS];  /* groups */                    
};

Great! I had this same idea yesterday evening a while after having left comments here. It's better like that.

Right, this diff was intended to do all of the mechanical conversions. You can see a prototype of the split that I've started preparing here: https://people.freebsd.org/~kevans/cr_gid.diff

I'll have a look, but a bit later. Just wanted to make sure you had a broader picture of the impacts.

I had considered doing all this after fixing some security problem where NFS would sometimes not fill cr_groups[] at all (so, the egid would not be initialized, and would end up being 0...), but more pressing stuff came in. Thanks for doing it!

In D51153#1167820, @kib wrote:

Did you considered writing a coccinelle script to do the transformation? We even have the place for it, tools/coccinelle.

I hadn't, mostly because it wasn't as bad as I expected and I wanted to take a look at each to make sure I documented assumptions about cr_gid in the surrounding area that would need fixed.

Whatever you plan, do you agree that the ABI properties of struct xucred must be intact?

Indeed. I did a little bit of thinking when I thought I'd need to bump the version (until I came up with a union-based solution that does what I want), but the existing users make this a lot more painful than it needs to be. Life would be a lot better if the version field was populated before userland asks kernel to populate one and we could take that as a request for an older version, but that ship has clearly sailed.

In D51153#1167640, @olce wrote:

In D51153#1167532, @kevans wrote:

My follow-up change that I'm toying with locally that actually splits cr_gid out solves this instead with some union trickery in struct xucred:

short   cr_ngroups;             /* number of groups (incl. cr_gid). */  
union {                                                                 
        /*                                                              
         * Special little hack to avoid needing a cr_gid macro, which   
         * would cause problems if one were to use it with struct ucred 
         * which also has a cr_groups member.                           
         */                                                             
        struct {                                                        
                gid_t   cr_gid;         /* effective group id */        
                gid_t   cr_sgroups[XU_NGROUPS - 1];                     
        };                                                              
                                                                        
        gid_t   cr_groups[XU_NGROUPS];  /* groups */                    
};

Great! I had this same idea yesterday evening a while after having left comments here. It's better like that.

Right, this diff was intended to do all of the mechanical conversions. You can see a prototype of the split that I've started preparing here: https://people.freebsd.org/~kevans/cr_gid.diff

Have you had a chance to look further at that patch? I'd like to land this one, then post an updated version of that diff for review as the next step (which immediately resolves all of the comments in this one)

In D51153#1172461, @kevans wrote:

Have you had a chance to look further at that patch? I'd like to land this one, then post an updated version of that diff for review as the next step (which immediately resolves all of the comments in this one)

Hi, no, was AFK the previous ~10 days. Will look at it soonish (tomorrow).