[PATCHv2] kvm: Fix nonsense handling of compat ioctl
From: Alan Cox 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 --- 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 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 --- 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
> 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: + 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: [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
> 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: [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 For the device allocation Acked-by: Alan Cox -- 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)
> > 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: [RFC PATCH] vhost-blk: An in-kernel accelerator for virtio-blk
On Thu, 11 Aug 2011 21:59:25 +0200 Dongsu Park 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 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 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 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 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.
> + 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 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-8&oe=utf-8&q=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] x86: default to reboot via ACPI
> Only common sense. Non-recent machines are barely usable these days. > Sure they work well as a firewall or server-in-a-closet, but if you run > a desktop or a server that actually does useful work, you're running a > relatively recent machine. Or a properly written desktop, or a thin client for desktop or ... The really old boxes are actually not the problem, we don't try and use ACPI on them so we won't try ACPI reboot. For those later ones it is probably worth trying anyway - we might end up with a different blacklist to before but there are multiple servers that really don't like old style reboot nowdays. 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] 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
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: Linux with kvm-intel locks up VMplayer guest is started
On Tue, 17 Jun 2008 15:32:34 +0200 Martin Michlmayr <[EMAIL PROTECTED]> wrote: > * Anthony Liguori <[EMAIL PROTECTED]> [2008-06-17 08:16]: > > VMware is a binary kernel module that's out of kernel. KVM is not > > misbehaving and the fact that VMware breaks when the KVM module is > > loaded isn't our problem. If they submitted their code for > > inclusion in mainline, we could possibly come up with solution for > > arbitrating who is using VT. > > I feared I'd get a response like this. But unless this is a known > issue in VMware (which I don't think it is), you don't know whether > it's not a bug in kvm-intel. So ask vmware. They have source to both parts we don't. And its a bug in vmware by definition. If the emulator differs from the real thing then the emulator is at fault. It might well be that the limit is a genuine technical limit - but its still an emulator fault. 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