[PATCHv2] kvm: Fix nonsense handling of compat ioctl
From: Alan Cox a...@linux.intel.com KVM_SET_SIGNAL_MASK passed a NULL argument leaves the on stack signal sets uninitialized. It then passes them through to kvm_vcpu_ioctl_set_sigmask. We should be passing a NULL in this case not translated garbage. Signed-off-by: Alan Cox a...@linux.intel.com --- virt/kvm/kvm_main.c |7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index a2e85af..cd1fde9a 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1975,9 +1975,10 @@ static long kvm_vcpu_compat_ioctl(struct file *filp, if (copy_from_user(csigset, sigmask_arg-sigset, sizeof csigset)) goto out; - } - sigset_from_compat(sigset, csigset); - r = kvm_vcpu_ioctl_set_sigmask(vcpu, sigset); + sigset_from_compat(sigset, csigset); + r = kvm_vcpu_ioctl_set_sigmask(vcpu, sigset); + } else + r = kvm_vcpu_ioctl_set_sigmask(vcpu, NULL); break; } default: -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND] kvm: Fix nonsense handling of compat ioctl
From: Alan Cox a...@linux.intel.com KVM_SET_SIGNAL_MASK passed a NULL argument leaves the on stack signal sets uninitialized. It then passes them through to kvm_vcpu_ioctl_set_sigmask. We should be passing a NULL in this case not translated garbage. Signed-off-by: Alan Cox a...@linux.intel.com --- virt/kvm/kvm_main.c |7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index a2e85af..e47a7ca 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1975,9 +1975,10 @@ static long kvm_vcpu_compat_ioctl(struct file *filp, if (copy_from_user(csigset, sigmask_arg-sigset, sizeof csigset)) goto out; - } - sigset_from_compat(sigset, csigset); - r = kvm_vcpu_ioctl_set_sigmask(vcpu, sigset); + sigset_from_compat(sigset, csigset); + r = kvm_vcpu_ioctl_set_sigmask(vcpu, sigset); + } else + kvm_vcpu_ioctl_set_sigmask(vcpu, NULL); break; } default: -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + ppc-e500_tlb-memset-clears-nothing.patch added to -mm tree
Yeah, not sure what's going on here. Alan sent me the patch a while back in a private mail, so I asked him to resend it to the ML, so it's available for review. Next thing that happens is this mail a few weeks later. I don't recall seeing your your reply and I really wasn't sure where it needed to go. I mostly sent it to PPC people. In the end I sent it to Andrew with a question as to where it should actually go. Either way, will bypass the normal process this time and pull it in my queue. It is certainly 3.6 material. Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: + ppc-e500_tlb-memset-clears-nothing.patch added to -mm tree
I wonder how many such bugs a memzero()/bzero() will prevent. If the compiler-foo is possible with gcc then a 0 length constant memset warning and a warning if the set value is 255 would both probably be useful. Fortunately a lot of other validation/verification tools do pick it up already. Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [RFC] Next gen kvm api
If the fd overhead really is a problem, perhaps the fd could be retained for setup operations, and omitted only on calls that require a vcpu to have been already set up on the current thread? Quite frankly I'd like to have an fd because it means you've got a meaningful way of ensuring that id reuse problems go away. You open a given id and keep a handle to it, if the id gets reused then your handle will be tied to the old one so you can fail the requests. Without an fd it's near impossible to get this right. The Unix/Linux model is open an object, use it, close it. I see no reason not to do that. Also the LSM hooks apply to file objects mostly, so its a natural fit on top *IF* you choose to use them. Finally you can pass file handles around between processes - do that any other way 8) Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [RFC] Next gen kvm api
register_pio_hook_ptr_r(PIO_IDE, SIZE_BYTE,s-cmd[0]);
for (i = 1; i 7; i++) {
register_pio_hook_ptr_r(PIO_IDE + i, SIZE_BYTE,s-cmd[i]);
register_pio_hook_ptr_w(PIO_IDE + i, SIZE_BYTE,s-cmd[i]);
}
You can't easily serialize updates to that address with the kernel since two
threads are likely going to be accessing it at the same time. That either
means
an expensive sync operation or a reliance on atomic instructions.
Who cares
If your API is right this isn't a problem (and for IDE the guess that it
won't happen you will win 99.999% of the time).
In fact IDE you can do even better in many cases because you'll get a
single rep outsw you can trap and shortcut.
But not all architectures offer non-word sized atomic instructions so it gets
fairly nasty in practice.
Thats their problem. We don't screwup the fast paths because some
hardware vendor screwed up that bit of their implementation. That's
*their* problem not everyone elses.
So on x86 IDE should be about 10 outb traps that can be predicted, a rep
outsw which can be shortcut and a completion set of inb/inw ops that can
be predicted.
You should hit userspace about once per IDE operation. Fix the hot paths
with good design and the noise doesn't matter.
Alan
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [Qemu-devel] [RFC] Next gen kvm api
register_pio_hook_ptr_r(PIO_IDE, SIZE_BYTE,s-cmd[0]);
for (i = 1; i 7; i++) {
register_pio_hook_ptr_r(PIO_IDE + i, SIZE_BYTE,s-cmd[i]);
register_pio_hook_ptr_w(PIO_IDE + i, SIZE_BYTE,s-cmd[i]);
}
You can't easily serialize updates to that address with the kernel since two
threads are likely going to be accessing it at the same time. That either
means
an expensive sync operation or a reliance on atomic instructions.
Who cares
If your API is right this isn't a problem (and for IDE the guess that it
won't happen you will win 99.999% of the time).
In fact IDE you can do even better in many cases because you'll get a
single rep outsw you can trap and shortcut.
But not all architectures offer non-word sized atomic instructions so it gets
fairly nasty in practice.
Thats their problem. We don't screwup the fast paths because some
hardware vendor screwed up that bit of their implementation. That's
*their* problem not everyone elses.
So on x86 IDE should be about 10 outb traps that can be predicted, a rep
outsw which can be shortcut and a completion set of inb/inw ops that can
be predicted.
You should hit userspace about once per IDE operation. Fix the hot paths
with good design and the noise doesn't matter.
Alan
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: add module alias (v2.1)
ACKs, NACKs? What is happening here? I would like an Ack from Alan Cox who switched vhost-net to a dynamic minor in the first place, in commit 79907d89c397b8bc2e05b347ec94e928ea919d33. Sorry dev...@lanana.org isn't yet back from the kernel hack incident. I don't read netdev so someone needs to summarise the issue and send me a copy of the patch to look at. Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] vhost-net: add module alias (v2.1)
Also: - use C99 style initialization. - add missing entry in documentation for loop-control Signed-off-by: Stephen Hemminger shemmin...@vyatta.com For the device allocation Acked-by: Alan Cox devi...@lanana.org -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] vhost-blk: An in-kernel accelerator for virtio-blk
On Thu, 11 Aug 2011 21:59:25 +0200
Dongsu Park dongsu.p...@profitbricks.com wrote:
Hi,
On 07/28/2011 05:22 PM, Michael S. Tsirkin wrote:
On Thu, Jul 28, 2011 at 10:29:05PM +0800, Liu Yuan wrote:
+static struct miscdevice vhost_blk_misc = {
+ 234,
Don't get a major unless you really must.
the minor number 234 conflicts with that of BTRFS, in kernel 3.0 at least.
Therefore you cannot load vhost_blk.ko if btrfs.ko was already loaded.
Probably that number should be something else, with which you don't have
conflict with any minor number in include/linux/miscdevice.h.
It should be zero or it should be officially reserved in devices.txt via
lan...@kernel.org, who (for it happens to be me currently) will turn it
down and tell you to use 0 to get a dynamic allocation unless you can
provide a very good reason that isn't suitable.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VFIO driver: Non-privileged user level PCI drivers
+/*
+ * Map usr buffer at specific IO virtual address
+ */
+static int vfio_dma_map_iova(
+ mlp = kzalloc(sizeof *mlp, GFP_KERNEL);
Not good at that point. I think you need to allocate it first, error if
it can't be allocated and then do the work and free it on error ?
+ mlp = kzalloc(sizeof *mlp, GFP_KERNEL);
+ mlp-pages = pages;
Ditto
+int vfio_enable_msix(struct vfio_dev *vdev, int nvec, void __user *uarg)
+{
+ struct pci_dev *pdev = vdev-pdev;
+ struct eventfd_ctx *ctx;
+ int ret = 0;
+ int i;
+ int fd;
+
+ vdev-msix = kzalloc(nvec * sizeof(struct msix_entry),
+ GFP_KERNEL);
+ vdev-ev_msix = kzalloc(nvec * sizeof(struct eventfd_ctx *),
+ GFP_KERNEL);
These don't seem to get freed on the error path - or indeed protected
against being allocated twice (eg two parallel ioctls ?)
+ case VFIO_DMA_MAP_ANYWHERE:
+ case VFIO_DMA_MAP_IOVA:
+ if (copy_from_user(dm, uarg, sizeof dm))
+ return -EFAULT;
+ ret = vfio_dma_map_common(listener, cmd, dm);
+ if (!ret copy_to_user(uarg, dm, sizeof dm))
So the vfio_dma_map is untrusted. That seems to be checked ok later but
the dma_map_common code then plays in current-mm- without apparently
holding any locks to stop the values getting corrupted by a parallel
mlock ?
Actually no I take that back
dmp-size is 64bit
So npage can end up with values like 0x and cause 32bit
boxes to go kerblam
+
+ case VFIO_EVENTFD_IRQ:
+ if (copy_from_user(fd, uarg, sizeof fd))
+ return -EFAULT;
+ if (vdev-ev_irq)
+ eventfd_ctx_put(vdev-ev_irq);
These paths need locking - suppose two EVENTFD irq ioctls occur at once
(in general these paths seem not to be covered)
+ case VFIO_BAR_LEN:
+ if (copy_from_user(bar, uarg, sizeof bar))
+ return -EFAULT;
+ if (bar 0 || bar PCI_ROM_RESOURCE)
+ return -EINVAL;
+ bar = pci_resource_len(pdev, bar);
+ if (copy_to_user(uarg, bar, sizeof bar))
+ return -EFAULT;
How does this all work out if the device is a bridge ?
+ pci_read_config_byte(pdev, PCI_INTERRUPT_LINE, line);
+ if (line == 0)
+ goto out;
That may produce some interestingly wrong answers. Firstly the platform
has interrupt abstraction so dev-irq may not match PCI_INTERRUPT_LINE,
secondly you have devices that report their IRQ via other paths as per
spec (notably IDE class devices in non-native mode)
So that would also want extra checks.
+ pci_read_config_word(pdev, PCI_COMMAND, orig);
+ ret = orig PCI_COMMAND_MASTER;
+ if (!ret) {
+ new = orig | PCI_COMMAND_MASTER;
+ pci_write_config_word(pdev, PCI_COMMAND, new);
+ pci_read_config_word(pdev, PCI_COMMAND, new);
+ ret = new PCI_COMMAND_MASTER;
+ pci_write_config_word(pdev, PCI_COMMAND, orig);
The master bit on some devices can be turned on but not off. Not sure it
matters here.
+ vdev-pdev = pdev;
Probably best to take/drop a reference. Not needed if you can prove your
last use is before the end of the remove path though.
Does look like it needs a locking audit, some memory and error checks
reviewing and some further review of the ioctl security and
overflows/trusted values.
Rather a nice way of attacking the user space PCI problem.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pci: allow sysfs file owner to read device dependent config space
On Wed, 12 May 2010 18:29:57 -0700
Chris Wright chr...@sous-sol.org wrote:
The PCI config space bin_attr read handler has a hardcoded CAP_SYS_ADMIN
check to verify privileges before allowing a user to read device
dependent config space. This is meant to protect from an unprivileged
user potentially locking up the box.
Or hacking it. You could argue the correct requirement is to change it to
require CAP_SYS_RAWIO in fact !
With this patch the sysfs file owner is also considered privileged enough
to read all of the config space.
Which breaks the main reason the check was there in the first place. To
stop accidents of the form
find / -exec grep {} wibble
blowing up in people's faces.
I agree with the problem - but IMHO the fix is to require opening the file
checks CAP_SYS_something instead: not to hack the read method and make it
even weirder and more un-Linux than it is now.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] Unify KVM kernel-space and user-space code into a single project
So it's good enough to be in Fedora/RHEL but not good enough to be in upstream glibc? How is that possible? Isnt that a double standard? Yes its a double standard Glibc has a higher standard than Fedora/RHEL. Just like the Ubuntu kernel ships various ugly unfit for upstream kernel drivers. kernel's fault. But glibc is certainly not being helpful in that situation either and your earlier claim that you are only waiting for the patches is rather dishonest. I am sure Ulrich is being totally honest, but send him the patches and you'll find out. Plus you will learn what the API should look like when you try and create them ... Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
The -next kernel no longer boots on kvm in 32bit mode
64bit Intel host system Fedora host environment 32bit guest built with CPU i686 and no PAE support This now momentarily flickers up something then reboots back to the BIOS. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Extending virtio_console to support multiple ports
- Then, are we certain that there's no case where the tty layer will call us with some lock held or in an atomic context ? To be honest, I've totally lost track of the locking rules in tty land lately so it might well be ok, but something to verify. Some of the less well behaved line disciplines do this and always have done. Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] add ksm kernel shared memory driver.
The minor number you are using already belongs to another project. 10,234 is free but it would be good to know what device naming is proposed. I imagine other folks would like to know why you aren't using sysfs or similar or extending /dev/kvm ? Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] deal with interrupt shadow state for emulated instruction
Why? Do you see a guest filling all of memory with 'mov ss' and expecting to break out of it via an interrupt? Well I did try mapping a page of move ss all through memory on real hardware long ago and seeing what happened on a 386 in real mode with DOSEMU. I was disappointed ;) -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 13/17] x86: allow the irq-vector translation to be determined outside of ioapic
On Tue, 31 Mar 2009 14:43:55 -0400 Gregory Haskins ghask...@novell.com wrote: The ioapic code currently privately manages the mapping between irq and vector. This results in some layering violations as the support for certain MSI operations need this info. As a result, the MSI code itself was moved to the ioapic module. This is not really optimal. This appears to have been muddled in with the vnet patches ? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver.
Taken off list Hmmm, list would like to know :-). That would be my choice too but unfortunately I can't do that -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver.
On Tue, 2 Dec 2008 10:07:24 -0800
Chris Wright [EMAIL PROTECTED] wrote:
* Alan Cox ([EMAIL PROTECTED]) wrote:
+ r = !memcmp(old_digest, sha1_item-sha1val, SHA1_DIGEST_SIZE);
+ mutex_unlock(sha1_lock);
+ if (r) {
+ char *old_addr, *new_addr;
+ old_addr = kmap_atomic(oldpage, KM_USER0);
+ new_addr = kmap_atomic(newpage, KM_USER1);
+ r = !memcmp(old_addr+PAGEHASH_LEN, new_addr+PAGEHASH_LEN,
+ PAGE_SIZE-PAGEHASH_LEN);
NAK - this isn't guaranteed to be robust so you could end up merging
different pages one provided by a malicious attacker.
I presume you're referring to the digest comparison. While there's
theoretical concern of hash collision, it's mitigated by hmac(sha1)
so the attacker can't brute force for known collisions.
Using current known techniques. A random collision is just as bad news.
This code simply isn't fit for the kernel.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver.
On Tue, 2 Dec 2008 13:24:11 -0800
Chris Wright [EMAIL PROTECTED] wrote:
* Alan Cox ([EMAIL PROTECTED]) wrote:
On Tue, 2 Dec 2008 10:07:24 -0800
Chris Wright [EMAIL PROTECTED] wrote:
* Alan Cox ([EMAIL PROTECTED]) wrote:
+ r = !memcmp(old_digest, sha1_item-sha1val, SHA1_DIGEST_SIZE);
+ mutex_unlock(sha1_lock);
+ if (r) {
+ char *old_addr, *new_addr;
+ old_addr = kmap_atomic(oldpage, KM_USER0);
+ new_addr = kmap_atomic(newpage, KM_USER1);
+ r = !memcmp(old_addr+PAGEHASH_LEN,
new_addr+PAGEHASH_LEN,
+ PAGE_SIZE-PAGEHASH_LEN);
NAK - this isn't guaranteed to be robust so you could end up merging
different pages one provided by a malicious attacker.
I presume you're referring to the digest comparison. While there's
theoretical concern of hash collision, it's mitigated by hmac(sha1)
so the attacker can't brute force for known collisions.
Using current known techniques. A random collision is just as bad news.
And, just to clarify, your concern would extend to any digest based
comparison? Or are you specifically concerned about sha1?
Taken off list
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/4] ksm - dynamic page sharing driver for linux v2
You have implemented second one, but seems it already was patented http://www.google.com/patents?vid=USPAT6789156 I'm not a lawyer but IMHO we have direct conflict here. From other point of view they have patented the WEEL, but at least we have to know about this. Its an old idea and appeared for Linux in March 1998: Little project from Philipp Reisner called mergemem. http://groups.google.com/group/muc.lists.linux-kernel/browse_thread/thread/387af278089c7066?ie=utf-8oe=utf-8q=share+identical+pages#b3d4f68fb5dd4f88 so if there is a patent which is relevant (and thats a question for lawyers and legal patent search people) perhaps the Linux Foundation and some of the patent busters could take a look at mergemem and re-examination. Alan -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] add ksm kernel shared memory driver.
+ r = !memcmp(old_digest, sha1_item-sha1val, SHA1_DIGEST_SIZE);
+ mutex_unlock(sha1_lock);
+ if (r) {
+ char *old_addr, *new_addr;
+ old_addr = kmap_atomic(oldpage, KM_USER0);
+ new_addr = kmap_atomic(newpage, KM_USER1);
+ r = !memcmp(old_addr+PAGEHASH_LEN, new_addr+PAGEHASH_LEN,
+ PAGE_SIZE-PAGEHASH_LEN);
NAK - this isn't guaranteed to be robust so you could end up merging
different pages one provided by a malicious attacker.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: default to reboot via ACPI
It seems excessive. Most machines will hardly run without acpi. *Recent* machines. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] x86: default to reboot via ACPI
Linux is used a lot in embedded systems (like routers and NAS-es and probably set-top boxes). I guess the # of embedded systems with some linux flavour on it is substantially bigger than the desktop systems. Most of these are ARM or MIPS based and do not support ACPI. They also don't support VT and are not x86 boxes so the code area involved doesn't even get compiled on them. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
