On Thu, Aug 05, 2010 at 06:40:57PM -0400, Oren Laadan wrote:
>
>
> On 08/05/2010 05:48 PM, Matt Helsley wrote:
> >On Thu, Aug 05, 2010 at 10:57:06AM -0700, Dan Smith wrote:
<snip>
> ><snip>
> >
> >>+static int restore_task_sem_undo_adj(struct ckpt_ctx *ctx)
>
> Now that Matt said it ... I suggest to rename this too to
> something like "restore_sem_undo_adj()" :)
>
> (Interestingly, the checkpoint counterpart is named so !)
>
> >>+{
> >>+ struct ckpt_hdr_task_sem_undo *h;
> >>+ int len;
> >>+ int ret = -ENOMEM;
> >>+ struct sem_undo *un;
> >>+ int nsems;
> >>+ __s16 *semadj = NULL;
> >>+
> >>+ h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_SEM_UNDO);
> >>+ if (IS_ERR(h))
> >>+ return PTR_ERR(h);
> >>+
> >>+ len = sizeof(__s16) * h->semadj_count;
> >>+ ret = ckpt_read_payload(ctx, (void **)&semadj, len, CKPT_HDR_BUFFER);
> >>+ if (ret< 0)
> >>+ goto out;
> >>+
> >
> >As best I can tell exit_sem() does not do permission checking -- it
> >relies on semop and semtimedop to check permissions on the operation
> >that created the undo in the first place. Thus I think we need to check
> >permissions here with ipc_check_perms() or ipcperms(). Otherwise it may
> >be possible to manipulate semaphores we do not have permission to
> >manipulate.
>
> Good catch !
Perhaps instead of mimicking the kernel data structure in the checkpoint image
and then restoring that kernel structure during restart we could create
a set of semops to write in the checkpoint image and then perform them
during restart. By effectively calling sys_semop during restart we would ensure
that we're doing the proper security checks, make the checkpoint image more
portable (since the semop interface can't change), and greatly reduce the
probability that c/r of the semaphores and their undo lists will bitrot
(IOW make maintenance easy for everyone).
Cheers,
-Matt Helsley
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
