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

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

On 07/10/13 15:49, Daniel Kiper wrote:
> On Mon, Oct 07, 2013 at 11:55:08AM +0100, David Vrabel wrote:
>> 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).
> 
> Once again, there is no UART code so there is no sens for this movq.
> Any smart developer (we have a dozens of them here) knows how to write
> relevant code. Now this movq only obfuscates things.

As a smart engineer I know I would much prefer to drop in debugging code
with minimal (risky) changes to existing code.

That said, I now have access to an ICE so I don't much care about
debugging with a UART so I'll make the suggested change.

>>>>>> +        /* 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.
> 
> I have never ever tried to define any ABI here. I have never ever said
> that the caller must pass this and the callee must expect that. There is
> no such definition in current Linux Kernel implementation too. Even purgatory
> expects nothing special in registers. I am just saying that it is worth to wipe
> data from GPRs. No more no less. If you would like to use any register to pass
> argument later you could do that. My proposal does not impose any limits.

There's an explicit ABI and an implicit one based on the implementation.
 In the absence of a formal test suite and certification program,
implicit ABI are often just as fixed and constraining as explicit ones.

>>>> 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?
> 
> I said "more difficult" not "prevent" and it makes difference.

You didn't actually answer my question.  What security issue do you
think zeroing registers will mitigate?  Given that the exec'd image has
full control over the host, access to all memory and all devices.

Lipstick on one pig in a whole herd of pigs comes to mind...

David