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

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

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.

> >>>> +        /* 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.

> >> 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.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel