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 06:44:17PM +0100, David Vrabel wrote:
> 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.

Still I am not convinced. This way you could add similar redundant stuff
in every piece of code, just in case, and finaly make it unreadable. Does
not make sens for me. Please remove this redundant movq or add UART code
or add relevant comment which explains why do you do that this way.
1 and 2 makes sens for me. 3 much less or at all.

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

Could you tell me where do you see an ABI here?

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

I am able to imagine a situation in which some registers values could
tell you how to put all interesting pieces together from somewhere.
Without them it could be difficult or even impossible in some cases.
It is difficult to point out a specific example now due to complexity
of Xen itself and other involved stuff. However, I belive that there
are smart guys in the wild with sufficient time and money to find some
(sooner or later). Why we would like to make their live easier?
Few extra instructions cost us nothing. For them this could be or
not to be.

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

I have never stated that this solve all security issues.
However, this way maybe we could avoid some problems in the future.

Daniel

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