From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andreas Herrmann Subject: Re: [PATCH 05/11] kvm tools, mips: Add MIPS support Date: Mon, 19 May 2014 15:37:56 +0200 Message-ID: <20140519133756.GA23153@alberich> References: <1399391491-5021-1-git-send-email-andreas.herrmann@caviumnetworks.com> <1399391491-5021-6-git-send-email-andreas.herrmann@caviumnetworks.com> <536D4571.6010302@imgtec.com> <20140512130110.GA17255@alberich> <5370D636.3020903@imgtec.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: Andreas Herrmann , Pekka Enberg , David Daney , kvm@vger.kernel.org, linux-mips@linux-mips.org, David Daney To: James Hogan Return-path: Received: from mail-ee0-f43.google.com ([74.125.83.43]:62396 "EHLO mail-ee0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753593AbaESNiC (ORCPT ); Mon, 19 May 2014 09:38:02 -0400 Received: by mail-ee0-f43.google.com with SMTP id d17so3593388eek.16 for ; Mon, 19 May 2014 06:38:00 -0700 (PDT) Content-Disposition: inline In-Reply-To: <5370D636.3020903@imgtec.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, May 12, 2014 at 03:09:58PM +0100, James Hogan wrote: > Hi Andreas, > > On 12/05/14 14:01, Andreas Herrmann wrote: > > On Fri, May 09, 2014 at 10:15:29PM +0100, James Hogan wrote: > >> On 06/05/14 16:51, Andreas Herrmann wrote: > >>> +static bool kvm_cpu__hypercall_write_cons(struct kvm_cpu *vcpu) > >>> +{ > >>> + int term = (int)vcpu->kvm_run->hypercall.args[0]; > >>> + u64 addr = vcpu->kvm_run->hypercall.args[1]; > >>> + int len = (int)vcpu->kvm_run->hypercall.args[2]; > >>> + char *host_addr; > >>> + > >>> + if (term < 0 || term >= TERM_MAX_DEVS) { > >>> + pr_warning("hypercall_write_cons term out of range <%d>", term); > >>> + return false; > >>> + } > >>> + if (len <= 0) { > >>> + pr_warning("hypercall_write_cons len out of range <%d>", len); > >>> + return false; > >>> + } > >>> + > >>> + if ((addr & 0xffffffffc0000000ull) == 0xffffffff80000000ull) > >>> + addr &= 0x1ffffffful; /* Convert KSEG{0,1} to physical. */ > >>> + if ((addr & 0xc000000000000000ull) == 0x8000000000000000ull) > >>> + addr &= 0x07ffffffffffffffull; /* Convert XKPHYS to pysical */ > >>> + > >>> + host_addr = guest_flat_to_host(vcpu->kvm, addr); > >>> + if (!host_addr) { > >>> + pr_warning("hypercall_write_cons unmapped physaddr %llx", (unsigned long long)addr); > >>> + return false; > >>> + } > >>> + > >>> + term_putc(host_addr, len, term); > >> > >> Does len need to be range checked? > > > > len <= 0 is checked above. > > I don't think an upper boundery check is required. > > term_putc (using write) should be able to handle it. > > No? > > Well it looks to me from my naive look at the code (my experience with > tools/kvm/ is pretty much just reading some of the code after looking at > this patchset) like the guest could provide a very large positive len > argument and overflow the host_addr of the memory bank, possibly reading > into other userspace memory which would then get written to the console. > Yes, if it's unmapped the kernel will detect it so it's not so bad (no > seg faults). I guess it all depends how any memory that is passed to > kvm__register_mem was allocated. mmap_anon_or_hugetlbfs may use mmap > which leaves the possibility open of another virtual mapping being > created immediately after it. > > AFAICT the best way to avoid that is probably to somehow extend > guest_flat_to_host to provide the address limit too so the provided > length can be checked/clipped, or maybe call it for the end address too > to check the full range is valid and belongs to the same mapping, > although that's a bit more of a hack and technically isn't watertight! > > Maybe I'm being paranoid though :) I aggree that also the upper bound should be checked. I think extending the len check with something like "|| !host_ptr_in_ram(vcpu->kvm,host_addr + len)" should do it. Thanks, Andreas