|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH RFC] xen/pvh: use a custom IO bitmap for PVH hardware domains
On 08/04/15 13:57, Roger Pau Monne wrote:
> Since a PVH hardware domain has access to the physical hardware create a
> custom more permissive IO bitmap. The permissions set on the bitmap are
> populated based on the contents of the ioports rangeset.
>
> Also add the IO ports of the serial console used by Xen to the list of not
> accessible IO ports.
Thankyou for looking into this. I think it is the correct general
direction, but I do have some questions/thoughts about this area.
I know that the current implementation is that dom0 is whitelisted and
can play with everything, but is this actually the best API?
Conceptually, a better approach would be for dom0 to start with no
permissions, and explicitly request access (After all, PV and PVH
domains are expected to know exactly what they are doing under Xen).
This has an extra advantage in that dom0 can't accidentally grant
permissions for resources it doens't know about to domUs.
Instead of adding to a growing blacklist in contruct_dom0, it might be a
better to maintain a global rangeset (or few) for resources which are
used by Xen and not permitted to be used by any other domains. This
would allow the ioports_deny_access()/etc calls to move into the correct
drivers, instead of having to extern things like the uart ports. It is
also far more likely to be kept up to date. (On that note, we could
probably do with an audit of the currently denied resources. I highly
doubt there is a PIT driver which could function with access to only
some of the ports).
In addition, some specific review...
>
> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>
> Cc: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@xxxxxxx>
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Eddie Dong <eddie.dong@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
> xen/arch/x86/domain_build.c | 10 ++++++++++
> xen/arch/x86/hvm/hvm.c | 11 +++++++++++
> xen/arch/x86/hvm/svm/vmcb.c | 3 ++-
> xen/arch/x86/hvm/vmx/vmcs.c | 6 ++++--
> xen/arch/x86/hvm/vmx/vmx.c | 1 +
> xen/drivers/char/ns16550.c | 10 ++++++++++
> xen/include/asm-x86/hvm/domain.h | 2 ++
> xen/include/asm-x86/hvm/hvm.h | 1 +
> xen/include/xen/serial.h | 4 ++++
> 9 files changed, 45 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index e5c845c..d0365fe 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -22,6 +22,7 @@
> #include <xen/compat.h>
> #include <xen/libelf.h>
> #include <xen/pfn.h>
> +#include <xen/serial.h>
> #include <asm/regs.h>
> #include <asm/system.h>
> #include <asm/io.h>
> @@ -1541,6 +1542,11 @@ int __init construct_dom0(
> rc |= ioports_deny_access(d, 0x40, 0x43);
> /* PIT Channel 2 / PC Speaker Control. */
> rc |= ioports_deny_access(d, 0x61, 0x61);
> + /* Serial console. */
> + if ( uart_ioport1 != 0 )
> + rc |= ioports_deny_access(d, uart_ioport1, uart_ioport1 + 7);
> + if ( uart_ioport2 != 0 )
> + rc |= ioports_deny_access(d, uart_ioport2, uart_ioport2 + 7);
> /* ACPI PM Timer. */
> if ( pmtmr_ioport )
> rc |= ioports_deny_access(d, pmtmr_ioport, pmtmr_ioport + 3);
> @@ -1618,6 +1624,10 @@ int __init construct_dom0(
>
> pvh_map_all_iomem(d, nr_pages);
> pvh_setup_e820(d, nr_pages);
> +
> + for ( i = 0; i < 0x10000; i++ )
> + if ( ioports_access_permitted(d, i, i) )
> + __clear_bit(i, hvm_hw_io_bitmap);
(There is surely a more efficient way of doing this? If there isn't,
there probably should be)
There is also a boundary issue between VT-x and SVM.
For VT-x, the IO bitmap is 2 pages. For SVM, it is 2 pages and 3 bits.
I suspect the difference is to do with the handling of a 4byte write to
port 0xffff. I think you might need to check "i < 0x10003" instead.
> }
>
> if ( d->domain_id == hardware_domid )
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 3ff87c6..6de89b2 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -82,6 +82,10 @@ struct hvm_function_table hvm_funcs __read_mostly;
> unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> hvm_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
>
> +/* I/O permission bitmap for HVM hardware domain */
> +unsigned long __attribute__ ((__section__ (".bss.page_aligned")))
> + hvm_hw_io_bitmap[3*PAGE_SIZE/BYTES_PER_LONG];
> +
> /* Xen command-line option to enable HAP */
> static bool_t __initdata opt_hap_enabled = 1;
> boolean_param("hap", opt_hap_enabled);
> @@ -162,6 +166,7 @@ static int __init hvm_enable(void)
> * often used for I/O delays, but the vmexits simply slow things down).
> */
> memset(hvm_io_bitmap, ~0, sizeof(hvm_io_bitmap));
> + memset(hvm_hw_io_bitmap, ~0, sizeof(hvm_hw_io_bitmap));
> if ( hvm_port80_allowed )
> __clear_bit(0x80, hvm_io_bitmap);
> __clear_bit(0xed, hvm_io_bitmap);
> @@ -1484,6 +1489,12 @@ int hvm_domain_initialise(struct domain *d)
> goto fail1;
> d->arch.hvm_domain.io_handler->num_slot = 0;
>
> + /* Set the default IO Bitmap */
> + if ( is_hardware_domain(d) )
> + d->arch.hvm_domain.io_bitmap = hvm_hw_io_bitmap;
> + else
> + d->arch.hvm_domain.io_bitmap = hvm_io_bitmap;
You will need to augment late_hwdom_init() as well, or you will break
anyone making us of "hardware_dom=$N"
~Andrew
> +
> if ( is_pvh_domain(d) )
> {
> register_portio_handler(d, 0, 0x10003, handle_pvh_io);
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 21292bb..eb2176b 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -72,6 +72,7 @@ static int construct_vmcb(struct vcpu *v)
> {
> struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
> struct vmcb_struct *vmcb = arch_svm->vmcb;
> + struct domain *d = v->domain;
>
> vmcb->_general1_intercepts =
> GENERAL1_INTERCEPT_INTR | GENERAL1_INTERCEPT_NMI |
> @@ -118,7 +119,7 @@ static int construct_vmcb(struct vcpu *v)
> svm_disable_intercept_for_msr(v, MSR_AMD64_LWP_CBADDR);
>
> vmcb->_msrpm_base_pa = (u64)virt_to_maddr(arch_svm->msrpm);
> - vmcb->_iopm_base_pa = (u64)virt_to_maddr(hvm_io_bitmap);
> + vmcb->_iopm_base_pa = (u64)virt_to_maddr(d->arch.hvm_domain.io_bitmap);
>
> /* Virtualise EFLAGS.IF and LAPIC TPR (CR8). */
> vmcb->_vintr.fields.intr_masking = 1;
> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
> index d614638..3080979 100644
> --- a/xen/arch/x86/hvm/vmx/vmcs.c
> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
> @@ -986,8 +986,10 @@ static int construct_vmcs(struct vcpu *v)
> }
>
> /* I/O access bitmap. */
> - __vmwrite(IO_BITMAP_A, virt_to_maddr((char *)hvm_io_bitmap + 0));
> - __vmwrite(IO_BITMAP_B, virt_to_maddr((char *)hvm_io_bitmap + PAGE_SIZE));
> + __vmwrite(IO_BITMAP_A,
> + virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap + 0));
> + __vmwrite(IO_BITMAP_B,
> + virt_to_maddr((char *)d->arch.hvm_domain.io_bitmap +
> PAGE_SIZE));
>
> if ( cpu_has_vmx_virtual_intr_delivery )
> {
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 2ac1492..d5ccdce 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3118,6 +3118,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> uint16_t port = (exit_qualification >> 16) & 0xFFFF;
> int bytes = (exit_qualification & 0x07) + 1;
> int dir = (exit_qualification & 0x08) ? IOREQ_READ : IOREQ_WRITE;
> +
> if ( handle_pio(port, bytes, dir) )
> update_guest_eip(); /* Safe: IN, OUT */
> }
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index d443880..b382e72 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -44,6 +44,9 @@ static char __initdata opt_com2[30] = "";
> string_param("com1", opt_com1);
> string_param("com2", opt_com2);
>
> +unsigned int uart_ioport1 = 0;
> +unsigned int uart_ioport2 = 0;
> +
> static struct ns16550 {
> int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
> u64 io_base; /* I/O port or memory-mapped I/O address. */
> @@ -1121,6 +1124,13 @@ void __init ns16550_init(int index, struct
> ns16550_defaults *defaults)
> uart->reg_shift = 0;
>
> ns16550_parse_port_config(uart, (index == 0) ? opt_com1 : opt_com2);
> + if ( uart->baud != 0 && uart->io_base < 0x10000 )
> + {
> + if ( index == 0 )
> + uart_ioport1 = uart->io_base;
> + else
> + uart_ioport2 = uart->io_base;
> + }
> }
>
> #ifdef HAS_DEVICE_TREE
> diff --git a/xen/include/asm-x86/hvm/domain.h
> b/xen/include/asm-x86/hvm/domain.h
> index 0702bf5..d002954 100644
> --- a/xen/include/asm-x86/hvm/domain.h
> +++ b/xen/include/asm-x86/hvm/domain.h
> @@ -143,6 +143,8 @@ struct hvm_domain {
> */
> uint64_t sync_tsc;
>
> + unsigned long *io_bitmap;
> +
> union {
> struct vmx_domain vmx;
> struct svm_domain svm;
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 0dc909b..55e432e 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -213,6 +213,7 @@ extern struct hvm_function_table hvm_funcs;
> extern bool_t hvm_enabled;
> extern bool_t cpu_has_lmsl;
> extern s8 hvm_port80_allowed;
> +extern unsigned long hvm_hw_io_bitmap[];
>
> extern const struct hvm_function_table *start_svm(void);
> extern const struct hvm_function_table *start_vmx(void);
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 71e6ade..bc5fb06 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -24,6 +24,10 @@ void serial_set_rx_handler(int handle, serial_rx_fn fn);
> /* Number of characters we buffer for an interrupt-driven transmitter. */
> extern unsigned int serial_txbufsz;
>
> +/* UART IO port */
> +extern unsigned int uart_ioport1;
> +extern unsigned int uart_ioport2;
> +
> struct uart_driver;
>
> enum serial_port_state {
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |