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
