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

[Pavel Emelyanov]

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.

> Does that sound right? I'll send a patch to fix LSM behavior to match
> the other creds behavior for now.

Yup :)

-- Pavel