On Wed, May 28, 2014 at 08:38:33AM -0700, Victor Kamensky wrote:
> On 28 May 2014 02:55, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:
> > On Tue, May 27, 2014 at 11:08:11PM -0700, Victor Kamensky wrote:
> >> Fix code that handles KVM_SET_ONE_REG, KVM_GET_ONE_REG ioctls to work in BE
> >> image. Before this fix get/set_one_reg functions worked correctly only in
> >> LE case - reg_from_user was taking 'void *' kernel address that actually could
> >> be target/source memory of either 4 bytes size or 8 bytes size, and code copied
> >> from/to user memory that could hold either 4 bytes register, 8 byte register
> >> or pair of 4 bytes registers.
> >>
> >> For example note that there was a case when 4 bytes register was read from
> >> userspace to kernel target address of 8 bytes value. Because it was working
> >> in LE, least significant word was memcpy(ied) and it just worked. In BE code
> >> with 'void *' as target/source 'val' type it is impossible to tell whether
> >> 4 bytes register from userspace should be copied to 'val' address itself
> >> (4 bytes target) or it should be copied to 'val' + 4 (least significant word
> >> of 8 bytes value). So first change was to introduce strongly typed
> >> functions, where type of target/source 'val' is strongly defined:
> >>
> >> reg_from_user_to_u64 - reads register from userspace to kernel 'u64 *val'
> >> address; register size must be 8 bytes
> >> reg_from_user_to_u32 - reads register(s) from userspace to kernel 'u32 *val'
> >> address; note it could be one or two 4 bytes registers
> >> reg_to_user_from_u64 - writes register from kernel 'u64 *val' address to
> >> userspace register memory; register size must be 8 bytes
> >> ret_to_user_from_u32 - writes register(s) from kernel 'u32 *val' address to
> >> userspace register(s) memory; note it could be
> >> one or two 4 bytes registers
> >>
> >> All places where reg_from_user, reg_to_user functions were used, were changed
> >> to use either corresponding u64 or u32 variants of functions depending on
> >> type of source/target kernel memory variable.
> >>
> >> Signed-off-by: Victor Kamensky <victor.kamensky@xxxxxxxxxx>
> >> ---
> >> arch/arm/kvm/coproc.c | 111 ++++++++++++++++++++++++++++++++++++++------------
> >> 1 file changed, 84 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/arch/arm/kvm/coproc.c b/arch/arm/kvm/coproc.c
> >> index c58a351..84d3ddb 100644
> >> --- a/arch/arm/kvm/coproc.c
> >> +++ b/arch/arm/kvm/coproc.c
> >> @@ -682,17 +682,60 @@ static struct coproc_reg invariant_cp15[] = {
> >> { CRn( 0), CRm( 0), Op1( 1), Op2( 7), is32, NULL, get_AIDR },
> >> };
> >>
> >> -static int reg_from_user(void *val, const void __user *uaddr, u64 id)
> >> +/*
> >> + * Reads a register value from a userspace address to a kernel u64
> >> + * variable. Register size must be always 8 bytes.
> >> + */
> >> +static int reg_from_user_to_u64(u64 *val, const void __user *uaddr, u64 id)
> >> +{
> >> + unsigned long regsize = KVM_REG_SIZE(id);
> >> +
> >> + if (regsize != 8)
> >> + return -ENOENT;
> >
> > no such file?
>
> What error code would you like me to use? I used ENOENT driven
> by existing examples in the same file like in
>
> static int demux_c15_set(u64 id, void __user *uaddr)
> ...
> if (KVM_REG_SIZE(id) != 4)
> return -ENOENT;
> ...
> static int vfp_get_reg(const struct kvm_vcpu *vcpu, u64 id, void __user *uaddr)
> {
> ...
> if (KVM_REG_SIZE(id) != 8)
> return -ENOENT;
>
hmm, maybe, I think those made more sence, because those were lookups
and didn't find any entry, but here it's more like someone just gave you
a wrong value to a particular function, so -EINVAL seemed more
appropriate. But I'm probably over-nit-picking.
> > This error check actually makes me even more convinced that we're
> > pushing the wrong functionality into this function, the _u64 function
> > should only be called if we have regsize == 8.
>
> Is not above check trying to enforce exactly what you said? _u64
> function will fail if it gets any other size than 8.
We shouldn't be writing defensive code in the kernel, we should call the
right functions.
>
> Not on the side I will try to rework _u32 and regsize == 8 along the
> lines you suggested: read into u64 using _u64 version, and than use
> something like vcpu_cp15_64_high, vcpu_cp15_64_low to put it
> into array of u32. In this case I will add
>
> if (KVM_REG_SIZE(id) != 4)
> return -ENOENT;
>
> to make sure _32 version used only with regsize == 32
>
> >
> >> +
> >> + if (copy_from_user(val, uaddr, regsize) != 0)
> >> + return -EFAULT;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/*
> >> + * Reads a register value from a userspace address to a kernel u32 variable
> >> + * or a kernel array of two u32 values. That is, it may really copy two
> >> + * u32 registers when used with registers defined by the ABI to be 8 bytes
> >> + * long (e.g. TTBR0/TTBR1).
> >> + */
> >> +static int reg_from_user_to_u32(u32 *val, const void __user *uaddr, u64 id)
> >> {
> >> - /* This Just Works because we are little endian. */
> >> if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
> >> return -EFAULT;
> >> return 0;
> >> }
> >>
> >> -static int reg_to_user(void __user *uaddr, const void *val, u64 id)
> >> +/*
> >> + * Writes a register value to a userspace address from a kernel u64 value.
> >> + * Register size must be always 8 bytes.
> >> + */
> >> +static int reg_to_user_from_u64(void __user *uaddr, const u64 *val, u64 id)
> >> +{
> >> + unsigned long regsize = KVM_REG_SIZE(id);
> >> +
> >> + if (regsize != 8)
> >> + return -ENOENT;
> >
> > same here
> >
> >> +
> >> + if (copy_to_user(uaddr, val, regsize) != 0)
> >> + return -EFAULT;
> >> + return 0;
> >> +}
> >
> > so yeah, we may not need multiple functions at all...
>
> I am not sure that I follow above completely, I think we would
> need to have separate _u32 and _u64 functions with proper
> checks inside to enforce rule that _u32 and kernel target
> type u32 is used with regsize == 4, and _u64 and kernel target
> type u64 is used with regsize == 8. Otherwise if it is only one
> function and it is called currently with the way we want, how
> can we make sure that in future these rules will stay the same?
>
> Or maybe we can use one macro that checks the passed
> target kernel type size matches regsize. In this case code will look
> nice and we would have enforcement we need. Let me try
> that.
>
Sounds good!
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
