[CRIU] [PATCH v2 1/3] seccomp: add initial support for SECCOMP_MODE_STRICT

[Tycho Andersen]

On Mon, Jun 08, 2015 at 10:43:57PM +0300, Pavel Emelyanov wrote:
> On 06/05/2015 08:10 PM, Tycho Andersen wrote:
> > On Fri, Jun 05, 2015 at 07:36:12PM +0300, Pavel Emelyanov wrote:
> >> On 06/05/2015 07:15 PM, Tycho Andersen wrote:
> >>> On Thu, Jun 04, 2015 at 03:34:07PM +0300, Pavel Emelyanov wrote:
> >>>>
> >>>>>>>  	}
> >>>>>>>  
> >>>>>>> @@ -1632,6 +1634,9 @@ static int attach_to_tasks(bool root_seized, enum trace_flags *flag)
> >>>>>>>  				return -1;
> >>>>>>>  			}
> >>>>>>>  
> >>>>>>> +			if (suspend_seccomp(pid) < 0)
> >>>>>>> +				return -1;
> >>>>>>
> >>>>>> BTW, why do we need to suspend seccomp on restore? The sigreturn is allowed in strict
> >>>>>> mode so if we just turn one ON before it nothing bad will (should) happen.
> >>>>>
> >>>>> Because the unmap of the parasite blob happens after this (in
> >>>>> finalize_restore() via ptrace), and if that isn't allowed the task is
> >>>>> killed.
> >>>>
> >>>> Ah, indeed :( But then things look even simpler and we don't need separate stage :)
> >>>>
> >>>> Look, the pie/restorer.c just turns seccomp on one line before calling sigreturn
> >>>> and does so. While doing this call it will be stopped up by criu with ptrace. And,
> >>>> once criu did this, it should check whether seccomp is ON, suspend it if required
> >>>> and continue task's execution.
> >>>>
> >>>> Does it make sense?
> >>>
> >>> ...and while fixing this, I realized that my poor understanding of
> >>> kernel threads meant that I'm only c/r-ing the seccomp state from the
> >>> first thread in the threadgroup, when in fact we should do it for all
> >>> of them (I'm looking in /proc/pid/status instead of
> >>> /proc/pid/task/tid/status, and only doing the restore once per pid
> >>> instead of per thread).
> >>>
> >>> This probably means that the lsm stuff is wrong too, since it seems
> >>> that can happen per thread as well, IIUC.
> >>
> >> It looks like the whole creds is ... not perfect in Linux http://ewontfix.com/17/ .
> > 
> > Sheesh. Quite the rabbit hole.
> > 
> >> From the CRIU perspective this means that creds.img should be applied to all threads
> >> in a group (which is true for most of the stuff, the restore_creds() is called by
> >> each thread on restore, need to fix that for LSM too). And, probably, some threads
> >> may have their own creds.img.
> > 
> > Yep. If my reading is correct, everything in the same thread group
> > uses the same creds.img (except for lsm, which needs to be fixed).
> > Since each thread can have its own creds, it seems like we need to fix
> > this to be per-thread? (Although probably in practice mostly they're
> > the same, so we could do some sort of de-duping, or only write a per
> > thread creds when it differs from the first thread's creds.)
> 
> I would add checks for creds for all threads being the same. Adding per-thread
> info could be done later if required.

Ok, sounds good. I will probably do that as a separate set; I have the
per-thread LSM patch and another version of the seccomp patches that I
will send in a bit.

Tycho