Re: Task server thread and task allocation/deallocation interfaces proposal

[Matthieu Lemerre]

Marcus Brinkmann <address@hidden> writes:
>
> I disagree.  We need task groups, so we can kill off child tasks which
> did not register themselves with the proc server.  This is not only
> important for sub-hurds like boot, or similar applications where a
> process doesn't register child tasks with the proc server, but also to
> clean up after a partial fork() or exec(), which otherwise could leave
> behind unregistered tasks in a broken state.
>
> The task group ID can just be a protected word that can only be set by
> a privileged server, for example proc, and which is inherited at task
> creation.  proc could set this word to something as simple as the PID.
>
> Killing a task should kill all tasks with the same task group ID, too.
> This ensures that no stranded unregistered tasks are left behind.

Shouldn't we instead pass a flag to tell if we want to destroy all
tasks with that group ID, or just this one?  If I think of a
particuliar application which create a new task and then wants to
destroy it, it must contact the proc server just to change the task id
of that task and make sure the task server won't destroy itself.

Eventually, tasks within the same group ID could be organized in a
tree structure, and when upon task destruction the task server would
kill all of its sons, recursively.  So killing the root of this tree
would kill all tasks.  It would also solve the problem below, without
changing the exec protocol.  On the implementation, I don't think that
it will make things more complicated, since I think that killing every
tasks on a group ID already require that the task_t are linked
together as a linked list.

I also wonder why is the group ID word useful.  Isn't links between
task_t nodes sufficient for storing relationships between the tasks?
task_set_group_id could be replaced by

task_reparent (l4_thread_id_t task_server,
               hurd_cap_handle_t parent_or_privileged_capability,
               hurd_cap_handle_t task_to_reparent)

and the new parent of TASK_TO_REPARENT would be set to
PARENT_OR_PRIVILEGED_CAPABILITY in case of it beeing a normal task
control capability, or there would be no parent if
PARENT_OR_PRIVILEGED_CAPABILITY is the proc control capability.
(This may be a bad interface).

Killing all tasks in a group ID also introduces the following problem:
if a tasks create a new task and give it to a filesystem, the latter,
if malicious, could kill both tasks (the passed one and the calling
one).  To avoid that, we should complete the exec protocol like this:
before giving a task control capability to the filesystem for a newly
created task, we have to contact the proc server to change the group
id of this task.

>
> So, yes, the task_create RPC should go to the "parent task"
>capability.  Furthermore, we need a privileged "task_set_group_id"
>RPC that sets the protected word.  And task_destroy should kill all
>tasks in the same group.
>
> BTW, one important thing to consider is how suid exec will work.  If
> we want proper accounting, suid tasks should be donated by the starter
> to the filesystem, but the filesystem needs total control over it.  So
> some revoke-and-reparent operation will be needed: revoke to remove
> access by the original starter, and reparent to associate the donated
> task to the filesystem doing the suid exec.  So, the task_set_group_id
> should also be permitted for non-privileged users in the special case
> that you want to set the group ID to the group ID of your own task
> (rather than any arbitrary group ID).  Ie, you claim a task you have
> control over (you have the task capability) as belonging to your own
> task group (there are no security problems with this I think - there
> _are_ security issues with donating your own tasks to somebody else's
> task group).
>

I think I found one of them ("a malicious filesystem could kill the
calling task."), and maybe a way to avoid it (the tree structure), but
I may miss many others.  Could you please explain a bit more on this?

Thanks,
Matthieu