lkml.org 
[lkml]   [2006]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] SELinux: Add security hook definition for getioprio and insert hooks
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-07-02 00:05    [W:0.036 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site