Re: [PATCH 05/11] kvm tools, mips: Add MIPS support
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 & 0xc000ull) == 0x8000ull)
> >>> + addr &= 0x1ffful; /* Convert KSEG{0,1} to physical. */
> >>> + if ((addr & 0xc000ull) == 0x8000ull)
> >>> + addr &= 0x07ffull; /* 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
--
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 05/11] kvm tools, mips: Add MIPS support
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 & 0xc000ull) == 0x8000ull)
>>> + addr &= 0x1ffful; /* Convert KSEG{0,1} to physical. */
>>> + if ((addr & 0xc000ull) == 0x8000ull)
>>> + addr &= 0x07ffull; /* 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
--
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 05/11] kvm tools, mips: Add MIPS support
On Fri, May 09, 2014 at 10:15:29PM +0100, James Hogan wrote:
> Hi Andreas,
>
> On 06/05/14 16:51, Andreas Herrmann wrote:
> > From: David Daney
> >
> > So far this was tested with host running KVM using MIPS-VZ (on Cavium
> > Octeon3). A paravirtualized mips kernel was used for the guest.
> >
> > [andreas.herrmann:
> >* Renamed kvm__arch_periodic_poll to kvm__arch_read_term
> > because of commit fa817d892508b6d3a90f478dbeedbe5583b14da7
> > (kvm tools: remove periodic tick in favour of a polling thread)
> >* Added ioport__map_irq skeleton to fix build problem.
> >* Rely on TERM_MAX_DEVS instead of using other macros
> >* Adaptions for MMIO support
> >* Set coalesc offset
> >* Fixed compile warnings]
> >
> > Signed-off-by: David Daney
> > Signed-off-by: Andreas Herrmann
>
>
> > +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 & 0xc000ull) == 0x8000ull)
> > + addr &= 0x1ffful; /* Convert KSEG{0,1} to physical. */
> > + if ((addr & 0xc000ull) == 0x8000ull)
> > + addr &= 0x07ffull; /* 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?
> > +void kvm_cpu__show_registers(struct kvm_cpu *vcpu)
> > +{
> > + struct kvm_regs regs;
> > +
> > + if (ioctl(vcpu->vcpu_fd, KVM_GET_REGS, ®s) < 0)
> > + die("KVM_GET_REGS failed");
> > + dprintf(debug_fd, "\n Registers:\n");
> > + dprintf(debug_fd, " --\n");
> > + dprintf(debug_fd, "$0 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[0], (unsigned long)regs.gpr[1],
> > + (unsigned long)regs.gpr[2], (unsigned long)regs.gpr[3]);
>
> Presumably there's nothing stopping a 32-bit userland from creating a
> 64-bit guest?
Yes, that can be run.
> If that's the case should this all use unsigned long longs?
... and yes it creates wrong register dump.
Will fix this.
> > + dprintf(debug_fd, "$4 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[4], (unsigned long)regs.gpr[5],
> > + (unsigned long)regs.gpr[6], (unsigned long)regs.gpr[7]);
> > + dprintf(debug_fd, "$8 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[8], (unsigned long)regs.gpr[9],
> > + (unsigned long)regs.gpr[10], (unsigned long)regs.gpr[11]);
> > + dprintf(debug_fd, "$12 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[12], (unsigned long)regs.gpr[13],
> > + (unsigned long)regs.gpr[14], (unsigned long)regs.gpr[15]);
> > + dprintf(debug_fd, "$16 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[16], (unsigned long)regs.gpr[17],
> > + (unsigned long)regs.gpr[18], (unsigned long)regs.gpr[19]);
> > + dprintf(debug_fd, "$20 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[20], (unsigned long)regs.gpr[21],
> > + (unsigned long)regs.gpr[22], (unsigned long)regs.gpr[23]);
> > + dprintf(debug_fd, "$24 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[24], (unsigned long)regs.gpr[25],
> > + (unsigned long)regs.gpr[26], (unsigned long)regs.gpr[27]);
> > + dprintf(debug_fd, "$28 : %016lx %016lx %016lx %016lx\n",
> > + (unsigned long)regs.gpr[28], (unsigned long)regs.gpr[29],
> > + (unsigned long)regs.gpr[30], (unsigned long)regs.gpr[31]);
> > +
> > + dprintf(debug_fd, "hi : %016lx\n", (unsigned long)regs.hi);
> > + dprintf(debug_fd, "lo : %016lx\n", (unsigned long)regs.lo);
> > + dprintf(debug_fd, "epc : %016lx\n", (unsigned long)regs.pc);
> > +
> > + dprintf(debug_fd, "\n");
> > +}
>
> Cheers
> James
Thanks,
Andreas
--
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 05/11] kvm tools, mips: Add MIPS support
Hi Andreas,
On 06/05/14 16:51, Andreas Herrmann wrote:
> From: David Daney
>
> So far this was tested with host running KVM using MIPS-VZ (on Cavium
> Octeon3). A paravirtualized mips kernel was used for the guest.
>
> [andreas.herrmann:
>* Renamed kvm__arch_periodic_poll to kvm__arch_read_term
> because of commit fa817d892508b6d3a90f478dbeedbe5583b14da7
> (kvm tools: remove periodic tick in favour of a polling thread)
>* Added ioport__map_irq skeleton to fix build problem.
>* Rely on TERM_MAX_DEVS instead of using other macros
>* Adaptions for MMIO support
>* Set coalesc offset
>* Fixed compile warnings]
>
> Signed-off-by: David Daney
> Signed-off-by: Andreas Herrmann
> +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 & 0xc000ull) == 0x8000ull)
> + addr &= 0x1ffful; /* Convert KSEG{0,1} to physical. */
> + if ((addr & 0xc000ull) == 0x8000ull)
> + addr &= 0x07ffull; /* 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?
> +void kvm_cpu__show_registers(struct kvm_cpu *vcpu)
> +{
> + struct kvm_regs regs;
> +
> + if (ioctl(vcpu->vcpu_fd, KVM_GET_REGS, ®s) < 0)
> + die("KVM_GET_REGS failed");
> + dprintf(debug_fd, "\n Registers:\n");
> + dprintf(debug_fd, " --\n");
> + dprintf(debug_fd, "$0 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[0], (unsigned long)regs.gpr[1],
> + (unsigned long)regs.gpr[2], (unsigned long)regs.gpr[3]);
Presumably there's nothing stopping a 32-bit userland from creating a
64-bit guest? If that's the case should this all use unsigned long longs?
> + dprintf(debug_fd, "$4 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[4], (unsigned long)regs.gpr[5],
> + (unsigned long)regs.gpr[6], (unsigned long)regs.gpr[7]);
> + dprintf(debug_fd, "$8 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[8], (unsigned long)regs.gpr[9],
> + (unsigned long)regs.gpr[10], (unsigned long)regs.gpr[11]);
> + dprintf(debug_fd, "$12 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[12], (unsigned long)regs.gpr[13],
> + (unsigned long)regs.gpr[14], (unsigned long)regs.gpr[15]);
> + dprintf(debug_fd, "$16 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[16], (unsigned long)regs.gpr[17],
> + (unsigned long)regs.gpr[18], (unsigned long)regs.gpr[19]);
> + dprintf(debug_fd, "$20 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[20], (unsigned long)regs.gpr[21],
> + (unsigned long)regs.gpr[22], (unsigned long)regs.gpr[23]);
> + dprintf(debug_fd, "$24 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[24], (unsigned long)regs.gpr[25],
> + (unsigned long)regs.gpr[26], (unsigned long)regs.gpr[27]);
> + dprintf(debug_fd, "$28 : %016lx %016lx %016lx %016lx\n",
> + (unsigned long)regs.gpr[28], (unsigned long)regs.gpr[29],
> + (unsigned long)regs.gpr[30], (unsigned long)regs.gpr[31]);
> +
> + dprintf(debug_fd, "hi : %016lx\n", (unsigned long)regs.hi);
> + dprintf(debug_fd, "lo : %016lx\n", (unsigned long)regs.lo);
> + dprintf(debug_fd, "epc : %016lx\n", (unsigned long)regs.pc);
> +
> + dprintf(debug_fd, "\n");
> +}
Cheers
James
--
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
