Re: [PATCH] SELinux: Add security hook definition for getioprio and insert hooks

["Serge E. Hallyn" <serue@us.ibm.com>]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> On Fri, 2006-06-30 at 14:37 -0500, Serge E. Hallyn wrote:
> > Quoting James Morris (jmorris@namei.org):
> > ...
> > > +static int get_task_ioprio(struct task_struct *p)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = security_task_getioprio(p);
> > > +	if (ret)
> > > +		goto out;
> > > +	ret = p->ioprio;
> > > +out:
> > > +	return ret;
> > > +}
> > ...
> > >  			do_each_task_pid(who, PIDTYPE_PGID, p) {
> > > +				tmpio = get_task_ioprio(p);
> > > +				if (tmpio < 0)
> > > +					continue;
> > >  				if (ret == -ESRCH)
> > > -					ret = p->ioprio;
> > > +					ret = tmpio;
> > >  				else
> > > -					ret = ioprio_best(ret, p->ioprio);
> > > +					ret = ioprio_best(ret, tmpio);
> > ...
> > > + * @task_getioprio
> > > + *	Check permission before getting the ioprio value of @p.
> > > + *	@p contains the task_struct of process.
> > > + *	Return 0 if permission is granted.
> > 
> > A return value >0 is a problem here but isn't mentioned.  the
> > get_task_ioprio() helper will return the the security_task_getioprio()
> > return value in htat case, but the do_each_task_pid loop will take it
> > as a valid return value.
> 
> True, but that isn't limited to that hook - think what happens if
> inode_permission returns an arbitrary positive integer. 

Well I don't know whether it's worth fixing for all of the lsm hooks,
maybe it is.  But in this case in particular there is just no reason for
it, and the fix is trivial...   Just switch the quoted parts to:

> +static int get_task_ioprio(struct task_struct *p)
> +{
> +	int ret;
> +
> +	ret = security_task_getioprio(p);
> +	if (ret<0)
> +		goto out;
> +	ret = p->ioprio;
> +out:
> +	return ret;
> +}
...
>  			do_each_task_pid(who, PIDTYPE_PGID, p) {
> +				tmpio = get_task_ioprio(p);
> +				if (tmpio < 0)
> +					continue;
>  				if (ret == -ESRCH)
> -					ret = p->ioprio;
> +					ret = tmpio;
>  				else
> -					ret = ioprio_best(ret, p->ioprio);
> +					ret = ioprio_best(ret, tmpio);
...
> + * @task_getioprio
> + *	Check permission before getting the ioprio value of @p.
> + *	@p contains the task_struct of process.
> + *	Return <0 if permission is denied, 0 if granted.

I'm not saying I'd expect this to be triggered, since most kernel and
lsm coders wouldn't think of returning >0 for error in any case...  Yet
you never know.  And again, there's just no reason for it.

-serge