Re: [PATCH 4/9] kexec: extend hypercall with improved load/unload ops

[David Vrabel <david.vrabel@citrix.com>]

On 07/10/13 11:39, Daniel Kiper wrote:
> On Mon, Oct 07, 2013 at 10:23:09AM +0100, David Vrabel wrote:
>> On 04/10/13 22:23, Daniel Kiper wrote:
>>> On Fri, Sep 20, 2013 at 02:10:50PM +0100, David Vrabel wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/x86_64/kexec_reloc.S
>>>> @@ -0,0 +1,208 @@
>> [...]
>>>> +ENTRY(kexec_reloc)
>>>> +        /* %rdi - code page maddr */
>>>> +        /* %rsi - page table maddr */
>>>> +        /* %rdx - indirection page maddr */
>>>> +        /* %rcx - entry maddr */
>>>> +        /* %r8 - flags */
>>>> +
>>>> +        movq %rdx, %rbx
>>>
>>> Delete movq %rdx, %rbx
>>
>> We avoid using %rdx in case we need to re-add the UART debugging.
> 
> Does not make sens for me. We could re-add it also if we remove this movq.
> Now it is not clear why it is here. I think that it should be removed.

outb uses %rdx so avoiding using %rdx means any UART debugging macros
are trivial (since they don't have to save/restore the value in %rdx).

>>>> +        /* Need to switch to 32-bit mode? */
>>>> +        testq $KEXEC_RELOC_FLAG_COMPAT, %r8
>>>> +        jnz call_32_bit
>>>> +
>>>> +call_64_bit:
>>>> +        /* Call the image entry point.  This should never return. */
>>>
>>> I think that all general purpose registers (including %rsi, %rdi, %rbp
>>> and %rsp) should be zeroed here. We should leave as little as possible
>>> info about previous system. Especially in kexec case. Just in case.
>>> Please look into linux/arch/x86/kernel/relocate_kernel_64.S
>>> for more details.
>>
>> Not initializing the registers is a deliberate design decision so exec'd
>> images cannot mistakenly rely on the register values.
> 
> Anybody who does this asks for problems. This is not our issue.

Zeroing the registers makes that part of the ABI for calling images,
which means it can never be changed.  If the ABI is the register values
are undefined then this can be changes in the future to something that
is defined.

>> Clearing a handful of words when all of host memory is accessible by the
>> exec'd image does nothing for security (as you suggest in a later email).
> 
> I am aware that this does not solve all security issues but it could make simple
> attacks more difficult.

What attacks?  What security issues is zero-ing a tiny amount of state
going to prevent when the exec'd image has full control over the whole host?

Your reasoning is completely bogus.

>> Linux uses call.
> 
> OK, to be precise in almost all cases it uses ret. It uses call if preserve
> context is enabled. However, I do not know who is using that feature (once
> someone considered even removal of this feature). So ret is main path to go
> to the new system image. Please check the sources.

I missed that which means I've definitely not changing this from the
obvious call.

David