Re: [PATCH v3 2/2] kvm: x86: hyperv: guest->host event signaling via eventfd

[David Hildenbrand <david@redhat.com>]

On 12.12.2017 10:02, Roman Kagan wrote:
> On Mon, Dec 11, 2017 at 10:14:48PM +0100, David Hildenbrand wrote:
>>
>>>  #endif /* __LINUX_KVM_H */
>>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
>>> index 015fb06c7522..0303bd8f4d6a 100644
>>> --- a/arch/x86/kvm/hyperv.c
>>> +++ b/arch/x86/kvm/hyperv.c
>>> @@ -29,6 +29,7 @@
>>>  #include <linux/kvm_host.h>
>>>  #include <linux/highmem.h>
>>>  #include <linux/sched/cputime.h>
>>> +#include <linux/eventfd.h>
>>>  
>>>  #include <asm/apicdef.h>
>>>  #include <trace/events/kvm.h>
>>> @@ -1226,6 +1227,54 @@ static int kvm_hv_hypercall_complete_userspace(struct kvm_vcpu *vcpu)
>>>  	return 1;
>>>  }
>>>  
>>> +static u16 hvcall_sigevent_param(struct kvm_vcpu *vcpu, gpa_t gpa, u64 *param)
>>> +{
>>> +	struct page *page;
>>> +	void *pg;
>>> +	u64 *msg;
>>> +
>>> +	if ((gpa & (__alignof__(*msg) - 1)) ||
>>> +	    offset_in_page(gpa) + sizeof(*msg) > PAGE_SIZE)
>>> +		return HV_STATUS_INVALID_ALIGNMENT;
>>> +
>>
>> Don't we also need srcu_read_lock(&vcpu->kvm->srcu) for
>> kvm_vcpu_gfn_to_page?
> 
> Seems so indeed.  I also think I'll switch to using
> kvm_vcpu_read_guest().

Probably better (because I think you also missed releasing the page)

> 
>>> +	page = kvm_vcpu_gfn_to_page(vcpu, gpa >> PAGE_SHIFT);
>>> +	if (is_error_page(page))
>>> +		return HV_STATUS_INSUFFICIENT_MEMORY;
>>> +
>>> +	pg = kmap_atomic(page);
>>> +	msg = pg + offset_in_page(gpa);
>>> +	*param = *msg;
>>> +	kunmap_atomic(pg);
>>> +	return HV_STATUS_SUCCESS;
>>> +}
>>> +
>>> +static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu, bool fast, u64 param)
>>> +{
>>> +	u16 ret;
>>> +	u32 conn_id;
>>> +	int idx;
>>> +	struct eventfd_ctx *eventfd;
>>> +
>>> +	if (unlikely(!fast)) {
>>> +		u64 gpa = param;
>>> +		ret = hvcall_sigevent_param(vcpu, gpa, &param);
>>> +		if (ret != HV_STATUS_SUCCESS)
>>> +			return ret;
>>> +	}
>>> +
>>> +	conn_id = (param & 0xffffffff) + ((param >> 32) & 0xffff);
>>
>> conn_id = param + ((param >> 32) & 0xffff);
>>
>> (as param will me automatically truncated to 32 bits - sizeof(conn_id))
> 
> Agreed.  However, it looks like what I really need is
> 
> conn_id = (param & KVM_HYPERV_CONN_ID_MASK) + ((param >> 32) & 0xffff);
> 
> otherwise the overflown conn_id may pass the following check

That would have been my next question :)

-- 

Thanks,

David / dhildenb