[CRIU] [PATCH] compel: compel_emergency_sigframe() helper introduced

[Dmitry Safonov]

2017-03-27 18:47 GMT+03:00 Stanislav Kinsburskiy <skinsbursky at virtuozzo.com>:
>
>
> 27.03.2017 17:13, Dmitry Safonov пишет:
>
>> 2017-03-27 17:25 GMT+03:00 Stanislav Kinsburskiy
>> <skinsbursky at virtuozzo.com>:
>>>
>>>
>>> 27.03.2017 16:07, Dmitry Safonov пишет:
>>>>
>>>> Yes, but fini_sigreturn() at this moment is called only for
>>>> parasite-based
>>>> infection. While the patch exports API for non-parasite case of
>>>> infection.
>>>> So, as I've asked before, can we complement it with usage of the formed
>>>> sigframe in the case of non-parasite infection?
>>>> It may be just a helper that has inside compel_syscall() with
>>>> rt_sigreturn.
>>>> I mean, if anyone is going to use API for forming sigframe, he will
>>>> have to write such helper. Maybe it's better to provide one for
>>>> integrity?
>>>>
>>> It's not that simple.
>>> User has to provide address *within* process address space, where
>>> sigframe
>>> must be placed, *to* sigframe creation routine.
>>> IOW, one need:
>>> 1) Some service mapping within process (doesn't exist in non-parasite
>>> case
>>> of infection), which he can use to store sigframe
>>> 2) Some way to call sigreturn from *within* the process (doesn't exist in
>>> non-parasite case of infection)
>>> 3) Calling to "rt_sigreturn" needs this sigframe address.
>>>
>>> Thus, from my POW, libcompel can to provide this helper "as is".
>>> Or this facility has to be significantly improved by hiding all this
>>> remote
>>> mapping creation, sigrame construction and rt_sigreturn call to some kind
>>> of
>>> helpers like below:
>>>
>>> int compel_put_some_straw(struct parasite_ctl *ctl);
>>>
>>> and paired (static, inlined, whatever else to prevent linking with
>>> libcompel):
>>>
>>> int compel_fall_from_the tree(void);
>>>
>> Well, I would vote for something like:
>>
>> compel_remote_mmap(struct parasite_ctl *ctl, unsigned long addr)
>> /* actually renamed compel_unmap() */
>> compel_remote_unmap(struct parasite_ctl *ctl, unsigned long addr)
>> compel_sigframe_prepare(struct rt_sigframe *raddr,
>>     user_regs_struct_t *regs, k_rtsigset_t* sig)
>> compel_sigframe_sigreturn(struct rt_sigframe *raddr)
>>
>> Maybe the naming could be improved.
>> This way we would have complete API for preparing sigframes for any
>> register set, blocked signals mask and for sigreturning to constructed
>> frames. All without parasite's help.
>>
>> Otherwise the original patch exports API for preparing sigframe only
>> at the point where an application was stopped and leaves reader's
>> exercise what to do with this sigframe afterward.
>>
>> What do you think, is it convenient API for your project?
>> I don't insist, but that looks logical to me avoiding inchoate APIs.
>>
>
> Well, you suggest to introduce 4 helpers. And add them to API.
> I suspect (almost sure) the following will happen:
> 1) Someone (or "The one", if you wish) will come and say, that names are bad
> and unacceptable.

They can be adjusted, but those two: remote_mmap() and remote_unmap()
I quite like because they say where they perform and what.

> 2) Then he will come once more and say, that "compel_remote_mmap/unmap" are
> stupid wrappers around compel_syscall(), which is ugly and unacceptable.

That's not only wrappers around compel_syscall() - they can do more,
e.g., providing safety for unmap() syscall by choosing another rx vma
to execute syscall/int80 instruction instead the one being unmapped.
(or using service vma). Which is all not needed for other syscalls
non-mmap-like.
(this is aside from syscall-in-vdso idea which I'll realize soon)

> And even if you somehow push this stuff, it won't be really valuable.
> Because there is no "fool protection" and user can easily shoot in his foot
> by passing any wrong address to compel_sigframe_prepare() as raddr,
> which will crash the tracee upon compel_sigframe_sigreturn() call.

Of cause the user can. That's why I would like to see a test/example
how to use new API.

> And one more thing to clarify, what you actually suggested:
> compel_sigframe_sigreturn() (or at least part of its logic) has to be
> executed in tracee.
> I see two ways to do this:
> 1) Link tracee with libcompel to take rt_sigreturn helper from there. "Link"
> here means load the library into tracee.
> 2) Be able to place some "rt_sigreturn" code to an arbitrary address within
> the process, and then jump to it.

I may be wrong, but IIRC, we can omit code within the process.
Having raddr with formatted sigframe - we put it into %sp, use
compel_syscall() and call compel_remote_unmap() for vma with raddr.

> So, yet again, I believe, that this API either has to be difficult to use
> (for those, who understand, what they are doing) or all the logic has to be
> hidden inside the library to prevent user from shooting himself in the foot.

-- 
             Dmitry