| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120815125915.GB3068@redhat.com>
Date: Wed, 15 Aug 2012 15:59:15 +0300
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Alex Williamson <alex.williamson@...hat.com>
Cc: avi@...hat.com, gleb@...hat.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/6] kvm: Expose IRQ source IDs to userspace
On Fri, Aug 10, 2012 at 04:37:25PM -0600, Alex Williamson wrote:
> Introduce KVM_IRQ_SOURCE_ID and KVM_CAP_NR_IRQ_SOURCE_ID to allow
> user allocation of IRQ source IDs and querying both the capability
> and the total count of IRQ source IDs. These will later be used
> by interfaces for setting up level IRQs.
>
> Signed-off-by: Alex Williamson <alex.williamson@...hat.com>
> ---
>
> Documentation/virtual/kvm/api.txt | 20 ++++++++++++++++++++
> arch/x86/kvm/Kconfig | 1 +
> arch/x86/kvm/x86.c | 3 +++
> include/linux/kvm.h | 11 +++++++++++
> include/linux/kvm_host.h | 1 +
> virt/kvm/Kconfig | 3 +++
> virt/kvm/irq_comm.c | 22 ++++++++++++++++++++++
> virt/kvm/kvm_main.c | 16 ++++++++++++++++
> 8 files changed, 77 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index bf33aaa..062cfd5 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1980,6 +1980,26 @@ return the hash table order in the parameter. (If the guest is using
> the virtualized real-mode area (VRMA) facility, the kernel will
> re-create the VMRA HPTEs on the next KVM_RUN of any vcpu.)
>
> +4.77 KVM_IRQ_SOURCE_ID
> +
> +Capability: KVM_CAP_NR_IRQ_SOURCE_ID
> +Architectures: x86
> +Type: vm ioctl
> +Parameters: struct kvm_irq_source_id (in/out)
> +Returns: 0 on success, -errno on error
> +
> +Allows allocating and freeing IRQ source IDs. Each IRQ source ID
> +represents a complete set of irqchip pin inputs which are logically
> +OR'd with other IRQ source IDs for determining the final assertion
> +level of a pin. The flag KVM_IRQ_SOURCE_ID_FLAG_DEASSIGN indicates
> +whether the call is for an allocation or deallocation.
> +kvm_irq_source_id.irq_source_id returns the allocated IRQ source ID
> +on success and specifies the freed IRQ source ID on deassign. The
> +return value of KVM_CAP_NR_IRQ_SOURCE_ID indicates the total number
> +of IRQ source IDs. These IDs are also shared with KVM internal users
> +(ex. KVM assigned devices, PIT, shared user ID), therefore not all IDs
> +may be allocated through this interface.
> +
> 5. The kvm_run structure
> ------------------------
> diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> index a28f338..bfd2082 100644
> --- a/arch/x86/kvm/Kconfig
> +++ b/arch/x86/kvm/Kconfig
> @@ -37,6 +37,7 @@ config KVM
> select TASK_DELAY_ACCT
> select PERF_EVENTS
> select HAVE_KVM_MSI
> + select HAVE_KVM_IRQ_SOURCE_ID
> ---help---
> Support hosting fully virtualized guest machines using hardware
> virtualization extensions. You will need a fairly recent
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 42bce48..75e743e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2209,6 +2209,9 @@ int kvm_dev_ioctl_check_extension(long ext)
> case KVM_CAP_TSC_DEADLINE_TIMER:
> r = boot_cpu_has(X86_FEATURE_TSC_DEADLINE_TIMER);
> break;
> + case KVM_CAP_NR_IRQ_SOURCE_ID:
> + r = BITS_PER_LONG; /* kvm->arch.irq_sources_bitmap */
> + break;
> default:
> r = 0;
> break;
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 2ce09aa..67b6b49 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -618,6 +618,7 @@ struct kvm_ppc_smmu_info {
> #define KVM_CAP_PPC_GET_SMMU_INFO 78
> #define KVM_CAP_S390_COW 79
> #define KVM_CAP_PPC_ALLOC_HTAB 80
> +#define KVM_CAP_NR_IRQ_SOURCE_ID 81
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> @@ -691,6 +692,14 @@ struct kvm_irqfd {
> __u8 pad[20];
> };
>
> +#define KVM_IRQ_SOURCE_ID_FLAG_DEASSIGN (1 << 0)
> +
> +struct kvm_irq_source_id {
> + __u32 flags;
> + __u32 irq_source_id;
> + __u8 pad[24];
> +};
> +
> struct kvm_clock_data {
> __u64 clock;
> __u32 flags;
> @@ -831,6 +840,8 @@ struct kvm_s390_ucas_mapping {
> #define KVM_PPC_GET_SMMU_INFO _IOR(KVMIO, 0xa6, struct kvm_ppc_smmu_info)
> /* Available with KVM_CAP_PPC_ALLOC_HTAB */
> #define KVM_PPC_ALLOCATE_HTAB _IOWR(KVMIO, 0xa7, __u32)
> +/* Available with KVM_CAP_IRQ_SOURCE_ID */
> +#define KVM_IRQ_SOURCE_ID _IOWR(KVMIO, 0xa8, struct kvm_irq_source_id)
>
> /*
> * ioctls for vcpu fds
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 2ad3e4a..ea6d7a1 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -636,6 +636,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm,
> struct kvm_irq_ack_notifier *kian);
> int kvm_request_irq_source_id(struct kvm *kvm);
> void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id);
> +int kvm_irq_source_id(struct kvm *kvm, struct kvm_irq_source_id *id);
>
> /* For vcpu->arch.iommu_flags */
> #define KVM_IOMMU_CACHE_COHERENCY 0x1
> diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
> index 28694f4..b7e0d4d 100644
> --- a/virt/kvm/Kconfig
> +++ b/virt/kvm/Kconfig
> @@ -21,3 +21,6 @@ config KVM_ASYNC_PF
>
> config HAVE_KVM_MSI
> bool
> +
> +config HAVE_KVM_IRQ_SOURCE_ID
> + bool
> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> index 7d75126..44d40c9 100644
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -254,6 +254,28 @@ unlock:
> mutex_unlock(&kvm->irq_lock);
> }
>
> +int kvm_irq_source_id(struct kvm *kvm, struct kvm_irq_source_id *id)
> +{
> + int irq_source_id;
> +
> + if (id->flags & ~KVM_IRQ_SOURCE_ID_FLAG_DEASSIGN)
> + return -EINVAL;
> +
> + if (id->flags & KVM_IRQ_SOURCE_ID_FLAG_DEASSIGN) {
> + if (id->irq_source_id == KVM_USERSPACE_IRQ_SOURCE_ID)
> + return -EINVAL;
This validation here is probably a work around
for assert in kvm_free_irq_source_id?
It would be good to add a comment explaining this,
or just drop assert in kvm_free_irq_source_id.
Also, kvm_free_irq_source_id prints error messages
if you give it an invalid value, so
this let userspace flood kernel log with error messages.
> + kvm_free_irq_source_id(kvm, id->irq_source_id);
> + return 0;
> + }
> +
Hmm. This will let buggy userspace deassign IDs such as PIT that it
never assigned. I think it will only break itself so it should be
harmless, but it would seem to work with subtle
interrupt-losing races, so maybe it would be nicer if we could
validate the ID was actually given to userspace at some point.
> + irq_source_id = kvm_request_irq_source_id(kvm);
kvm_request_irq_source_id was not intended to be called
by userspace so it behaves a bit strangely:
- If you call this too many times you let userspace flood
kernel log with warnings
- return value is -EFAULT which is ignored by another caller
but passed to userspace here, arguably wrong
> + if (irq_source_id < 0)
> + return irq_source_id;
> +
> + id->irq_source_id = irq_source_id;
> + return 0;
> +}
> +
There is a minor bug here: if you call this too many times then following
PIT and assigned device allocation will fail.
This is in contrast to documentation which says that
getting source id will fail in this case,
and generally gives userspace no way to detect
what went wrong and recover: it only sees ENOMEM.
One solution I see is split the source ID space, give half to userspace
and half to internal users.
> void kvm_register_irq_mask_notifier(struct kvm *kvm, int irq,
> struct kvm_irq_mask_notifier *kimn)
> {
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2468523..e120cb3 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2093,6 +2093,22 @@ static long kvm_vm_ioctl(struct file *filp,
> break;
> }
> #endif
> +#ifdef CONFIG_HAVE_KVM_IRQ_SOURCE_ID
> + case KVM_IRQ_SOURCE_ID: {
> + struct kvm_irq_source_id id;
> +
> + r = -EFAULT;
> + if (copy_from_user(&id, argp, sizeof(id)))
> + goto out;
> +
> + r = kvm_irq_source_id(kvm, &id);
> + if (!r && copy_to_user(argp, &id, sizeof(id))) {
> + r = -EFAULT;
> + goto out;
> + }
> + break;
> + }
> +#endif
> default:
> r = kvm_arch_vm_ioctl(filp, ioctl, arg);
> if (r == -ENOTTY)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists