Re: [PATCH 05/11] kvm tools, mips: Add MIPS support

[James Hogan <james.hogan@imgtec.com>]

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 :)

Cheers
James