On 23.04.2010, at 12:17, Fernando Luis Vázquez Cao wrote:

quoted text
> On 04/23/2010 08:29 AM, Alexander Graf wrote:
>> 
>> On 22.04.2010, at 08:09, Fernando Luis Vázquez Cao wrote:
>> 
>>> On 04/22/2010 11:45 AM, Fernando Luis Vázquez Cao wrote:
>>>> On 04/21/2010 06:41 PM, Alexander Graf wrote:
>>>>> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
>>>>> 
>>>>>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>>>>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>>>>>> 	__u32 padding1;
>>>>>>> 	union {
>>>>>>> 		void __user *dirty_bitmap; /* one bit per page */
>>>>>>> -		__u64 padding2;
>>>>>>> +		__u64 addr;
>>>>>> 
>>>>>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
>>>>> 
>>>>> So the high 32 bits are zero. Where's the problem?
>>>> 
>>>> If we are careful enough to cast the addr appropriately we should be fine,
>>>> even if we keep the padding field in the union. I am not saying that it
>>>> breaks 32 architectures but that it can potentially be problematic.
>>>> 
>>>>>>> +	case KVM_SWITCH_DIRTY_LOG: {
>>>>>>> +		struct kvm_dirty_log log;
>>>>>>> +
>>>>>>> +		r = -EFAULT;
>>>>>>> +		if (copy_from_user(&log, argp, sizeof log))
>>>>>>> +			goto out;
>>>>>>> +		r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>>>>>> +		if (r)
>>>>>>> +			goto out;
>>>>>>> +		r = -EFAULT;
>>>>>>> +		if (copy_to_user(argp, &log, sizeof log))
>>>>>>> +			goto out;
>>>>>>> +		r = 0;
>>>>>>> +		break;
>>>>>>> +	}
>>>>>> 
>>>>>> In x86_64-compat mode we are handling 32bit user-space addresses
>>>>>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
>>>>> 
>>>>> The compat code just forwards everything to the generic ioctls.
>>>> 
>>>> The compat code uses struct compat_kvm_dirty_log instead of
>>>> struct kvm_dirty_log to communicate with user space so
>>>> the necessary conversions needs to be done before invoking
>>>> the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).
>>>> 
>>>> By the way we probable should move the definition of struct
>>>> compat_kvm_dirty_log to a header file.
>>> 
>>> It seems that it was you and Arnd who added the kvm_vm compat ioctl :-).
>>> Are you considering a different approach to tackle the issues that we
>>> have with a big-endian userspace?
>> 
>> IIRC the issue was a pointer inside of a nested structure, no?
> 
> I would say the reason is that if we did not convert the user-space pointer to
> a "void *" kvm_get_dirty_log() would end up copying the dirty log to
> 
> (log->dirty_bitmap << 32) | 0x00000000

Well yes, that was the problem. If we always set the __u64 value to the pointer we're safe though.

union {
  void *p;
  __u64 q;
}

void x(void *r)
{
  // breaks:
  p = r;

  // works:
  q = (ulong)r;
}

Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html