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

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

> 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. I think that it costs nothing. Linux does this.
Why we could not do that?

> >> +        callq *%rcx
> >
> > Maybe we should use retq to jump into image entry point. If not
> > I think that we should store image entry point address in %rax
> > (just to the order).
>
> With call if an image does try to return it will fault at a well defined
> point (the following ud2) making the failure a bit easier to diagnose.
> With your suggestion it will fault somewhere random.

As I said I do not insist on changing that. I know why that you did this
in that way. ret is just an option to consider. That is all.

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

Daniel

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