x86/entry: Fix passing 6th argument for compat hypercalls
PATCH 1/1 x86/entry: Fix passing 6th argument for compat hypercalls
Commit ec05090403ef4d760fbe701e31afd0f0edc414d5 ("x86/entry: Erase guest GPR state on entry to Xen") zero-ed %rbp, compat arg 6, but it is not restored before passing to hypercalls. We need to pass the saved compat arg 6 to the hypercall in r9, the 6th function argument.
---
This is against Xen 4.6. I believe it is also applicable to 4.7.
xen/arch/x86/x86_64/compat/entry.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
xen/arch/x86/x86_64/compat/entry.S +1 -1
@@ -56,7 +56,7 @@ UNLIKELY_END(msi_check) xchgl %ecx,%esi /* Arg 2, Arg 4 */ movl %edx,%edx /* Arg 3 */ movl %edi,%r8d /* Arg 5 */ - movl %ebp,%r9d /* Arg 6 */ + movl UREGS_rbp(%rsp),%r9d /* Arg 6 */ movl UREGS_rbx(%rsp),%edi /* Arg 1 */ #define SHADOW_BYTES 0 /* No on-stack shadow state */ #endif
Signature · 7 lines
2.14.3 _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 7 lines · Jason Andryuk
Commit
ec05090403ef4d760fbe701e31afd0f0edc414d5("x86/entry: Erase guest GPR state on entry to Xen") zero-ed %rbp, compat arg 6, but it is not restored before passing to hypercalls. We need to pass the saved compat arg 6 to the hypercall in r9, the 6th function argument.Signed-off-by Jason Andryuk <[email protected]>---
Jan: for reference, this is precisely the reason why I recommended that you backport the minimal 51e5d6c7a29 which I provided, rather than try to fix up the existing on-stack logic.
Looking at that commit, there is another bug in LOAD_C_CLOBBERED, where passing compat=0 xc=0 corrupts %rcx with whatever was on the stack for %r10
Quoted 10 lines
This is against Xen 4.6. I believe it is also applicable to 4.7.
xen/arch/x86/x86_64/compat/entry.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)xen/arch/x86/x86_64/compat/entry.S +0
-0
xen/arch/x86/x86_64/compat/entry.S UNLIKELY_END(msi_check) · line 60
Quoted 6 lines
xen/arch/x86/x86_64/compat/entry.S +1
-1@@ -56,7 +56,7 @@ UNLIKELY_END(msi_check) xchgl %ecx,%esi /* Arg 2, Arg 4 */ movl %edx,%edx /* Arg 3 */ movl %edi,%r8d /* Arg 5 */ - movl %ebp,%r9d /* Arg 6 */ + movl UREGS_rbp(%rsp),%r9d /* Arg 6 */ movl UREGS_rbx(%rsp),%edi /* Arg 1 */
This looks correct, (but there is a good reason why I purged this code from the codebase 3 releases ago).
~Andrew
xen/arch/x86/x86_64/compat/entry.S UNLIKELY_END(msi_check) · line 62
Quoted 2 lines
xen/arch/x86/x86_64/compat/entry.S +0
-0#define SHADOW_BYTES 0 /* No on-stack shadow state */ #endif
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 7 lines
Quoted 1 line
Quoted 1 line · On 16.03.18 at 21:08, <[email protected]> wrote:
Quoted 5 lines · Jason Andryuk
Commit
ec05090403ef4d760fbe701e31afd0f0edc414d5("x86/entry: Erase guest GPR state on entry to Xen") zero-ed %rbp, compat arg 6, but it is not restored before passing to hypercalls. We need to pass the saved compat arg 6 to the hypercall in r9, the 6th function argument.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Jason Andryuk <[email protected]>---
Jan: for reference, this is precisely the reason why I recommended that you backport the minimal
51e5d6c7a29which I provided, rather than try to fix up the existing on-stack logic.
And I continue to disagree that infrastructure changes should generally be backported. There's always going to be _some_ point where this can't (reasonably) be done anymore (e.g. without backporting further and further infrastructure changes), so better not to do it at all.
Quoted 2 lines
Looking at that commit, there is another bug in
LOAD_C_CLOBBERED, where passing compat=0 xc=0 corrupts %rcx with whatever was on the stack for %r10
That's intended behavior, albeit perhaps not immediately obvious without looking at the change the original commit does to xen/arch/x86/x86_64/entry.S: It replaces a move from %r10 to %rcx by a LOAD_C_CLOBBERED invocation. For these old trees I decided that's an acceptable thing to do, even if perhaps I wouldn't want to see something like that in master.
Quoted 1 line
Quoted 1 line
This is against Xen 4.6. I believe it is also applicable to 4.7.
Yes, I'll put this into both.
Quoted 17 lines
Quoted 8 lines
xen/arch/x86/x86_64/compat/entry.S | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)xen/arch/x86/x86_64/compat/entry.S +0
-0xen/arch/x86/x86_64/compat/entry.S UNLIKELY_END(msi_check) · line 60
Quoted 6 lines
xen/arch/x86/x86_64/compat/entry.S +1
-1@@ -56,7 +56,7 @@ UNLIKELY_END(msi_check) xchgl %ecx,%esi /* Arg 2, Arg 4 */ movl %edx,%edx /* Arg 3 */ movl %edi,%r8d /* Arg 5 */ - movl %ebp,%r9d /* Arg 6 */ + movl UREGS_rbp(%rsp),%r9d /* Arg 6 */ movl UREGS_rbx(%rsp),%edi /* Arg 1 */This looks correct, (but there is a good reason why I purged this code from the codebase 3 releases ago).
Right, yet independent of your purging of it perhaps most fundamentally the point that iirc there are no 6-argument hypercalls in the first place.
Jan
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel