|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86: correct create_bounce_frame
On 02/05/17 14:22, Jan Beulich wrote:
> Commit d9b7ef209a7 ("x86: drop failsafe callback invocation from
> assembly") didn't go quite far enough with the cleanup it did: The
> changed maximum frame size should also have been reflected in the early
> address range check (which has now been pointed out to have been wrong
> anyway, using 60 instead of 0x60), and it should have updated the
> comment ahead of the function.
>
> Also adjust the lower bound - all is fine (for our purposes) if the
> initial guest kernel stack pointer points right at the hypervisor base
> address, as only memory _below_ that address is going to be written.
I'm not sure this a useful change to make. HYPERVISOR_VIRT_START has at
minimum page alignment, and we have just aligned %rsi down to a 16 byte
boundary.
>
> Additionally limit the number of times %rsi is being adjusted to what
> is really needed.
>
> Finally move exception fixup code into the designated .fixup section.
>
> Reported-by: Jann Horn <jannh@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> This corrects the code which did result in XSA-215 on Xen 4.6 and
> older. For that reason I at least want to explore whether this is a
> change we want to take for 4.9.
I think it is certainly worth taking, if only to correct the comment.
>
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -258,7 +258,7 @@ int80_slow_path:
> jmp handle_exception_saved
>
> /* CREATE A BASIC EXCEPTION FRAME ON GUEST OS STACK: */
> -/* { RCX, R11, [DS-GS,] [CR2,] [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
> +/* { RCX, R11, [ERRCODE,] RIP, CS, RFLAGS, RSP, SS } */
> /* %rdx: trap_bounce, %rbx: struct vcpu */
> /* On return only %rbx and %rdx are guaranteed non-clobbered. */
> create_bounce_frame:
> @@ -276,9 +276,9 @@ create_bounce_frame:
> movq UREGS_rsp+8(%rsp),%rsi
> andb $0xfc,UREGS_cs+8(%rsp) # Indicate kernel context to guest.
> 2: andq $~0xf,%rsi # Stack frames are 16-byte aligned.
> - movq $HYPERVISOR_VIRT_START,%rax
> + movq $HYPERVISOR_VIRT_START+1,%rax
> cmpq %rax,%rsi
> - movq $HYPERVISOR_VIRT_END+60,%rax
> + movq $HYPERVISOR_VIRT_END+8*8,%rax
> sbb %ecx,%ecx # In +ve address space? Then okay.
> cmpq %rax,%rsi
> adc %ecx,%ecx # Above Xen private area? Then okay.
> @@ -286,13 +286,13 @@ UNLIKELY_START(g, create_bounce_frame_ba
> lea UNLIKELY_DISPATCH_LABEL(create_bounce_frame_bad_sp)(%rip), %rdi
> jmp asm_domain_crash_synchronous /* Does not return */
> __UNLIKELY_END(create_bounce_frame_bad_sp)
> - subq $40,%rsi
> + subq $7*8,%rsi
> movq UREGS_ss+8(%rsp),%rax
> ASM_STAC
> movq VCPU_domain(%rbx),%rdi
> -.Lft2: movq %rax,32(%rsi) # SS
> +.Lft2: movq %rax,6*8(%rsi) # SS
> movq UREGS_rsp+8(%rsp),%rax
> -.Lft3: movq %rax,24(%rsi) # RSP
> +.Lft3: movq %rax,5*8(%rsi) # RSP
> movq VCPU_vcpu_info(%rbx),%rax
> pushq VCPUINFO_upcall_mask(%rax)
> testb $TBF_INTERRUPT,TRAPBOUNCE_flags(%rdx)
> @@ -301,7 +301,7 @@ __UNLIKELY_END(create_bounce_frame_bad_s
> popq %rax
> shlq $32,%rax # Bits 32-39: saved_upcall_mask
> movw UREGS_cs+8(%rsp),%ax # Bits 0-15: CS
> -.Lft4: movq %rax,8(%rsi) # CS / saved_upcall_mask
> +.Lft4: movq %rax,3*8(%rsi) # CS / saved_upcall_mask
> shrq $32,%rax
> testb $0xFF,%al # Bits 0-7: saved_upcall_mask
> setz %ch # %ch == !saved_upcall_mask
> @@ -313,16 +313,15 @@ __UNLIKELY_END(create_bounce_frame_bad_s
> testb $1 << VMASST_TYPE_architectural_iopl,DOMAIN_vm_assist(%rdi)
> cmovnzl VCPU_iopl(%rbx),%ecx # Bits 13:12 (EFLAGS.IOPL)
> orl %ecx,%eax # Fold EFLAGS.IOPL into %eax
> -.Lft5: movq %rax,16(%rsi) # RFLAGS
> +.Lft5: movq %rax,4*8(%rsi) # RFLAGS
> movq UREGS_rip+8(%rsp),%rax
> -.Lft6: movq %rax,(%rsi) # RIP
> +.Lft6: movq %rax,2*8(%rsi) # RIP
> testb $TBF_EXCEPTION_ERRCODE,TRAPBOUNCE_flags(%rdx)
> jz 1f
> subq $8,%rsi
> movl TRAPBOUNCE_error_code(%rdx),%eax
> -.Lft7: movq %rax,(%rsi) # ERROR CODE
> +.Lft7: movq %rax,2*8(%rsi) # ERROR CODE
> 1:
> - subq $16,%rsi
> movq UREGS_r11+8(%rsp),%rax
> .Lft12: movq %rax,8(%rsi) # R11
For consistency, this should become movq %rax, 1*8(%rsi) and later
0*8(%rsi) for .Lft13, so all deltas from (%rsi) are expressed in terms
of slots.
> movq UREGS_rcx+8(%rsp),%rax
> @@ -345,15 +344,20 @@ UNLIKELY_START(z, create_bounce_frame_ba
> __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
> movq %rax,UREGS_rip+8(%rsp)
> ret
> - _ASM_EXTABLE(.Lft2, domain_crash_page_fault_32)
> - _ASM_EXTABLE(.Lft3, domain_crash_page_fault_24)
> - _ASM_EXTABLE(.Lft4, domain_crash_page_fault_8)
> - _ASM_EXTABLE(.Lft5, domain_crash_page_fault_16)
> - _ASM_EXTABLE(.Lft6, domain_crash_page_fault)
> - _ASM_EXTABLE(.Lft7, domain_crash_page_fault)
> + _ASM_EXTABLE(.Lft2, domain_crash_page_fault_48)
> + _ASM_EXTABLE(.Lft3, domain_crash_page_fault_40)
> + _ASM_EXTABLE(.Lft4, domain_crash_page_fault_24)
> + _ASM_EXTABLE(.Lft5, domain_crash_page_fault_32)
> + _ASM_EXTABLE(.Lft6, domain_crash_page_fault_16)
> + _ASM_EXTABLE(.Lft7, domain_crash_page_fault_16)
> _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
Given that you have altered the notation for a delta from (%rsi) (which
is a good improvement), these labels should be similarly altered for
consistency.
~Andrew
> _ASM_EXTABLE(.Lft13, domain_crash_page_fault)
>
> + .pushsection .fixup, "ax", @progbits
> +domain_crash_page_fault_48:
> + addq $8,%rsi
> +domain_crash_page_fault_40:
> + addq $8,%rsi
> domain_crash_page_fault_32:
> addq $8,%rsi
> domain_crash_page_fault_24:
> @@ -380,6 +384,7 @@ ENTRY(dom_crash_sync_extable)
> orb %al,UREGS_cs(%rsp)
> xorl %edi,%edi
> jmp asm_domain_crash_synchronous /* Does not return */
> + .popsection
>
> ENTRY(common_interrupt)
> SAVE_ALL CLAC
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |