Re: [PATCH 2/2] Add support for the per-task sem_undo list (v2)

[Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org>]

On 08/04/2010 04:17 PM, Dan Smith wrote:
>>> +static void put_undo_list(struct sem_undo_list *ulp);
>
> OL>  nit: to me it makes more sense to move
> OL>  grab/drop/checkpoint/restart code, as well sem_init(), to the end
> OL>  of the file (and avoid those statics...).
>
> Okay, was trying to avoid chopping the file up too much.
>
> OL>  Can get rid of ..._users() since we don't collect them.
>
> Oops :)
>
> OL>  semadj is short, so:
> OL>  s/__u16/__s16/
>
> Gah, yeah, I always want everything to be unsigned for some reason :D
>
>>> -		sma = sem_lock_check(tsk->nsproxy->ipc_ns, un->semid);
>>> +		sma = sem_lock_check(ulp->ipc_ns, un->semid);
>
> OL>  This hunk belongs to previous patch, no ?
>
> It could, but it seems more relevant when converting the function from
> using a task to using just the undo_list itself...
>
>>> +	atomic_inc(&ulp->refcnt);
>
> OL>  I suspect there is a leak here: atomic_inc is needed only for
> OL>  tasks other than the first task; The first task already gets the
> OL>  refcount deep inside find_alloc_undo(), and the hash-table gets
> OL>  its ref via the obj->grab method.
>
> But then it drops that ref right after it inserts it, thus we need
> another one, right?.  I think the confusion may come from the fact
> that the hash assumes the first refcount is its own and the caller of
> restore_obj() will grab another (for the task, in this case).  Since
> we don't do that, we need to grab both early on.  I hit this in
> another place in the network stuff, IIRC, which generated a similar
> review comment ;)

True ... but -

In that case, the atomic_inc for the first task should occur
earlier - as soon as it is attached to the task _and_ inserted
into the objhash.

Consider the following scenario:  the task calls find_alloc_undo(),
so the undo_list is attached to the task; then when the restore
succeeds, the undo_list is also in the obj_hash. But one reference
is missing. A malicious user can make restart fail now - e.g. by
corrupting the image file - before the first tasks grabbed the
additional reference :(

So my point is valid if not accurate - the atomic_inc() does not
belong there.

---

And this made me think... I wonder if the following is a security
hazard for us:

In the current code, e.g. restore_file_table(), the first task
that restores a given file-table (or mm) will assume that it has
a "fresh" file-table (or mm) and will modify it on-the-spot.

This works because userspace restart will arrange for restarting
tasks to be like that (only threads will a-priori share mm). If
not, then a later task could "overwrite" the restore work of a
previous task in case they had shared e.g. file-table when the
restart starts.

That in itself is not a concern; but support a malicious user
modifies userspace restart, to have multiple tasks share their
file-table and/or mm. That user could arrange for the image to
force a certain state (file-table, mm, ..) on a previous task
when restoring a later task.

Again, in itself it is not a concern; but what if that previous
task was restored to have more privileges/credentials ?  would
that allow a malicious user to break out ?

Oren.