[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [patch] sys_setpriority() euid semantics fix
Ingo Molnar wrote:
* Peter Williams <pwil3058@xxxxxxxxxxxxxx> wrote:
I've just noticed what might be a bug in the original code. Shouldn't
if ((current->euid != p->euid) && (current->euid != p->uid) &&
if ((current->uid != p->uid) && (current->euid != p->uid) &&
I.e. if the real or effective uid of the task doing the setting is not
the same as the uid of the target task it is not allowed to change the
target task's policy unless it is specially privileged.
no. The original code is quite logical: when doing something to others,
only the euid is taken into account. When others do something to you,
both the uid and the euid is checked ('others' might have no idea about
this task temporarily changing the euid to a less/more privileged uid).
So sys_setscheduler() [and sys_setaffinity(), which does the same] is
I disagree. Logically, the only privileges that should be relevant are
those of the task making the change i.e. those of current. The
effective uid of the target task is irrelevant. If only the effective
uid counts for determining what a task can do then the statement can be
if ((current->euid != p->uid) && !capable(CAP_SYS_NICE))
but I think that this is wrong as having an effective uid that is
different to the real uid doesn't cancel the privileges assoctaied with
the real uid.
The way uid and effective uid effect a task's operations on files are a
guide to their logical application in cases like this.
Since this operation is usually performed by a task on itself it
probably doesn't matter. But when task A is trying to change the policy
of task B then it is the privileges of the task A that count and all
that is relevant about task B is who its real owner is.
For example, if a user A ran a program that was setuid to an
unprivileged user B then A would be unable (from within that program) to
change the policy of any RT tasks that she had running to SCHED_NORMAL
unless those tasks were also setuid to B. I agree that this is a highly
unlikely circumstance but it does serve to illustrate the point w.r.t.
what is logical.
what _is_ inconsistent is kernel/sys.c's setpriority()/set_one_prio().
It checks current->euid|uid against p->uid, which makes little sense,
I think that this is correct.
but is how we've been doing it ever since. It's a Linux quirk documented
in the manpage. To make things funnier, SuS requires current->euid|uid
match against p->euid.
I've just read the man page and I think that the description of what
Linux does (or is supposed to do) is correct/logical and that the other
behaviors described are (very) illogical. In any case, the change that
I suggested will make the behaviour match what the man page says.
The patch below fixes it (and brings the logic in line with what
setscheduler()/setaffinity() does), but if we do it then it should be
done only in 2.6.12 or later, after good exposure in -mm.
(Worst-case this could break an application but i highly doubt it: it at
most could deny renicing another task to positive (or in very rare
cases, to negative) nice values, which no application should crash on
something like that, normally.)
Yes, it's fairly benign. Especially as most common use of this would be
tasks calling it on themselves. I'm not going to lose any sleep over it :-)
Peter Williams pwil3058@xxxxxxxxxxxxxx
"Learning, n. The kind of ignorance distinguishing the studious."
-- Ambrose Bierce
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/