From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v2] x86/ioemul: Rewrite stub generation to be shadow stack compatible
Date: Fri, 22 May 2020 16:03:13 +0200 [thread overview]
Message-ID: <502e32d2-5e4c-7c84-417e-7b1e44a0d8b4@suse.com> (raw)
In-Reply-To: <20200522102435.14329-1-andrew.cooper3@citrix.com>
On 22.05.2020 12:24, Andrew Cooper wrote:
> --- a/xen/arch/x86/ioport_emulate.c
> +++ b/xen/arch/x86/ioport_emulate.c
> @@ -8,7 +8,10 @@
> #include <xen/sched.h>
> #include <xen/dmi.h>
>
> -static bool ioemul_handle_proliant_quirk(
> +unsigned int __read_mostly (*ioemul_handle_quirk)(
I guess the more correct (and long term supported) placement is
unsigned int (*__read_mostly ioemul_handle_quirk)(
as you mean to modify the variable, not its type.
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -47,51 +47,97 @@ struct priv_op_ctxt {
> unsigned int bpmatch;
> };
>
> -/* I/O emulation support. Helper routines for, and type of, the stack stub. */
> -void host_to_guest_gpr_switch(struct cpu_user_regs *);
> -unsigned long guest_to_host_gpr_switch(unsigned long);
> +/* I/O emulation helpers. Use non-standard calling conventions. */
> +void nocall load_guest_gprs(struct cpu_user_regs *);
> +void nocall save_guest_gprs(void);
>
> typedef void io_emul_stub_t(struct cpu_user_regs *);
>
> static io_emul_stub_t *io_emul_stub_setup(struct priv_op_ctxt *ctxt, u8 opcode,
> unsigned int port, unsigned int bytes)
> {
> + /*
> + * Construct a stub for IN/OUT emulation.
> + *
> + * Some platform drivers communicate with the SMM handler using GPRs as a
> + * mailbox. Therefore, we must perform the emulation with the hardware
> + * domain's registers in view.
> + *
> + * We write a stub of the following form, using the guest load/save
> + * helpers (non-standard ABI), and one of several possible stubs
> + * performing the real I/O.
> + */
> + static const char prologue[] = {
> + 0x53, /* push %rbx */
> + 0x55, /* push %rbp */
> + 0x41, 0x54, /* push %r12 */
> + 0x41, 0x55, /* push %r13 */
> + 0x41, 0x56, /* push %r14 */
> + 0x41, 0x57, /* push %r15 */
> + 0x57, /* push %rdi (param for save_guest_gprs) */
> + }; /* call load_guest_gprs */
> + /* <I/O stub> */
> + /* call save_guest_gprs */
> + static const char epilogue[] = {
> + 0x5f, /* pop %rdi */
> + 0x41, 0x5f, /* pop %r15 */
> + 0x41, 0x5e, /* pop %r14 */
> + 0x41, 0x5d, /* pop %r13 */
> + 0x41, 0x5c, /* pop %r12 */
> + 0x5d, /* pop %rbp */
> + 0x5b, /* pop %rbx */
> + 0xc3, /* ret */
> + };
> +
> struct stubs *this_stubs = &this_cpu(stubs);
> unsigned long stub_va = this_stubs->addr + STUB_BUF_SIZE / 2;
> - long disp;
> - bool use_quirk_stub = false;
> + unsigned int quirk_bytes = 0;
> + char *p;
> +
> + /* Helpers - Read outer scope but only modify p. */
> +#define APPEND_BUFF(b) ({ memcpy(p, b, sizeof(b)); p += sizeof(b); })
> +#define APPEND_CALL(f) \
> + ({ \
> + long disp = (long)(f) - (stub_va + p - ctxt->io_emul_stub + 5); \
> + BUG_ON((int32_t)disp != disp); \
> + *p++ = 0xe8; \
> + *(int32_t *)p = disp; p += 4; \
> + })
>
> if ( !ctxt->io_emul_stub )
> ctxt->io_emul_stub =
> map_domain_page(_mfn(this_stubs->mfn)) + (stub_va & ~PAGE_MASK);
>
> - /* call host_to_guest_gpr_switch */
> - ctxt->io_emul_stub[0] = 0xe8;
> - disp = (long)host_to_guest_gpr_switch - (stub_va + 5);
> - BUG_ON((int32_t)disp != disp);
> - *(int32_t *)&ctxt->io_emul_stub[1] = disp;
> + p = ctxt->io_emul_stub;
> +
> + APPEND_BUFF(prologue);
> + APPEND_CALL(load_guest_gprs);
>
> + /* Some platforms might need to quirk the stub for specific inputs. */
> if ( unlikely(ioemul_handle_quirk) )
> - use_quirk_stub = ioemul_handle_quirk(opcode, &ctxt->io_emul_stub[5],
> - ctxt->ctxt.regs);
> + {
> + quirk_bytes = ioemul_handle_quirk(opcode, p, ctxt->ctxt.regs);
> + p += quirk_bytes;
> + }
>
> - if ( !use_quirk_stub )
> + /* Default I/O stub. */
> + if ( likely(!quirk_bytes) )
> {
> - /* data16 or nop */
> - ctxt->io_emul_stub[5] = (bytes != 2) ? 0x90 : 0x66;
> - /* <io-access opcode> */
> - ctxt->io_emul_stub[6] = opcode;
> - /* imm8 or nop */
> - ctxt->io_emul_stub[7] = !(opcode & 8) ? port : 0x90;
> - /* ret (jumps to guest_to_host_gpr_switch) */
> - ctxt->io_emul_stub[8] = 0xc3;
> + *p++ = (bytes != 2) ? 0x90 : 0x66; /* data16 or nop */
> + *p++ = opcode; /* <opcode> */
> + *p++ = !(opcode & 8) ? port : 0x90; /* imm8 or nop */
> }
>
> - BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX(9, /* Default emul stub */
> - 5 + IOEMUL_QUIRK_STUB_BYTES));
> + APPEND_CALL(save_guest_gprs);
> + APPEND_BUFF(epilogue);
> +
> + BUG_ON(STUB_BUF_SIZE / 2 < (p - ctxt->io_emul_stub));
I continue to view this as a dis-improvement, because a bug here
may go unnoticed for a very long time, until someone runs Xen
on one of the affected Proliants _and_ invokes an action
triggering the quirk code. I don't see any better alternative
than what I think I had already described. Hence preferably with
the remark further up addressed
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
prev parent reply other threads:[~2020-05-22 14:10 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-22 10:24 [PATCH v2] x86/ioemul: Rewrite stub generation to be shadow stack compatible Andrew Cooper
2020-05-22 14:03 ` Jan Beulich [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=502e32d2-5e4c-7c84-417e-7b1e44a0d8b4@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).