SE Linux patch(Re: [BusyBox] SE Linux)

[Rob Landley]

On Friday 04 February 2005 05:13 pm, Takeharu KATO wrote:
> Hi John:
>
> Sorry,I found a bug the patch I send before.
> The patch break compilation of top command of busybox in the case of
> selinux support off.
>
> Please use the patch attached with this mail.

Okay, since this came up again...  I dont' use selinux, but I do commit things 
to the busybox repository (until Erik screams about it, anyway), so lemme 
read the patch...

> >  #ifdef CONFIG_SELINUX
> > -	if(is_flask_enabled_flag) {

In theory when the whole #ifdef is surrounded by an #if statement like that, 
the #ifdef #endif could be replaced with a #define hidden in a header 
somewhere that allows the compiler's dead code elimination to handle things.  
Just a comment...

> >  #ifdef CONFIG_SELINUX
> > -	while ((p = procps_scan(0, 0, NULL)) != 0) {
> > +	security_context_t sid=NULL;
> > +	while ((p = procps_scan(0, 0,&sid)) != 0) {
> >  #else
> >  	while ((p = procps_scan(0)) != 0) {
> >  #endif

Changing curly bracket nesting levels in a #ifdef is a bit disturbing.  (Aside 
from confusing some syntax aware editors...)

> >  #ifdef CONFIG_SELINUX
> >  		if(use_selinux)
> >  		{
> > -			if(fstat_secure(fileno(fp), &sb, sid))
> > -				continue;
> > +			if (is_selinux_enabled()) {
> > +					if (getpidcon(pid,sid)<0)
> > +							continue;
> > +			}
> >  		}
> > -		else
> >  #endif

Again the if() wrapped #ifdef...

> >  	syslog(LOG_INFO, "System Maintenance Mode\n");
> > -	run_shell(pwent.pw_shell, 1, 0, 0);
> > +#ifdef CONFIG_SELINUX
> > +	getcon(&sid);
> > +#endif
> > +	run_shell(pwent.pw_shell, 1, 0, 0
> > +#ifdef CONFIG_SELINUX
> > +	, sid
> > +#endif
> > +		  );
> >  	return (0);
> >  }

This part makes my eyeballs hurt.  You're changing the number of arguments of 
run_shell depending on a config option.  Is there some way to make sid a 
global, or have it re-query it or something?

Oh the whole, I suppose it doesn't look worse than what's already there.  It's 
fairly intrusive #ifdeffery and looks like the whole thing could use a 
cleanup pass, but that's not new in this patch...

If there's consensus that this is ok, I can apply it...

Rob