On Wed, 15 Aug 2012 18:25:24 +0100, Peter Maydell <peter.maydell@xxxxxxxxxx> wrote:
> On 6 August 2012 08:36, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
> > +static int copy_reg_to_user(const struct coproc_reg *reg, u32 __user **uind)
> > +{
> > + if (!*uind)
> > + return 0;
> > +
> > + if (put_user(cp15_to_index(reg), *uind))
> > + return -EFAULT;
> > +
> > + (*uind)++;
> > + return 0;
> > +}
>
> This function returns 0 on success or a negative errno on failure...
Erk. Fix below.
> > +/* Assumed ordered tables, see kvm_coproc_table_init. */
> > +static int walk_msrs(struct kvm_vcpu *vcpu, u32 __user *uind)
> > +{
> > + const struct coproc_reg *i1, *i2, *end1, *end2;
> > + unsigned int total = 0;
> > + size_t num;
> > +
> > + /* We check for duplicates here, to allow arch-specific overrides. */
> > + i1 = get_target_table(vcpu->arch.target, &num);
> > + end1 = i1 + num;
> > + i2 = cp15_regs;
> > + end2 = cp15_regs + ARRAY_SIZE(cp15_regs);
> > +
> > + BUG_ON(i1 == end1 || i2 == end2);
> > +
> > + /* Walk carefully, as both tables may refer to the same register. */
> > + while (i1 && i2) {
> > + int cmp = cmp_reg(i1, i2);
> > + /* target-specific overrides generic entry. */
> > + if (cmp <= 0) {
> > + /* Ignore registers we trap but don't save. */
> > + if (i1->reg) {
> > + if (!copy_reg_to_user(i1, &uind))
> > + return -EFAULT;
>
> ...but this code assumes it returns 0 on failure
>
> > + total++;
> > + }
> > + } else {
> > + /* Ignore registers we trap but don't save. */
> > + if (i2->reg) {
> > + if (!copy_reg_to_user(i2, &uind))
> > + return -EFAULT;
>
> ...as does this.
>
> The effect is that
> (a) trying to query the length of the list returns a too small
> or even negative number since kvm_num_guest_msrs() doesn't notice
> it just got handed an errno
>
> (b) trying to actually read the list always fails -EFAULT.
>
> Dropping the "!"s fixes this for me, but you might prefer to
> change the return value of copy_reg_to_user() instead.
Yech.... My "testing" after rewriting this code was to check that qemu
launced a guest, but qemu seems to ignore the error return, so I didn't
even notice that it was completely broken :(
I have added cp15 API tests to my test branch (they were on the TODO,
this jumped them to the top). Indeed, it shows this as broken.
It also shows that KVM_VCPU_GET_MSR_INDEX_LIST returns the number of
msrs on success, not 0 as the documentation says (and x86 does).
Patches coming.
Thanks,
Rusty, wearing the brown bag of shame.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
