Re: [PATCH 5/8] KVM: Check IOPL level during io instruction emulation.

[Avi Kivity <avi@redhat.com>]

On 02/09/2010 04:14 PM, Gleb Natapov wrote:
> Make emulator check that vcpu is allowed to execute IN, INS, OUT,
> OUTS, CLI, STI.
>
>
>
> +bool kvm_check_iopl(struct kvm_vcpu *vcpu)
> +{
> +	int iopl;
> +	if (!is_protmode(vcpu))
> +		return false;
> +	if (kvm_get_rflags(vcpu)&  X86_EFLAGS_VM)
> +		return true;
> +	iopl = (kvm_get_rflags(vcpu)&  X86_EFLAGS_IOPL)>>  IOPL_SHIFT;
> +	return kvm_x86_ops->get_cpl(vcpu)>  iopl;
> +}
>    

Confusingly named -  check doesn't imply what the return value means 
(and 'true' is surprising for a failure).  Suggest kvm_bad_iopl() or 
similar.

> +
> +bool kvm_check_io_port_access_allowed(struct kvm_vcpu *vcpu, u16 port, u16 len)
> +{
>    

Similarly, can drop check_ from the name.

> +	struct kvm_segment tr_seg;
> +	int r;
> +	u16 io_bitmap_ptr;
> +	u8 perm, bit_idx = port&  0x7;
> +	unsigned mask = (1<<  len) - 1;
> +
> +	kvm_get_segment(vcpu,&tr_seg, VCPU_SREG_TR);
> +	if (tr_seg.unusable)
> +		return false;
> +	if (tr_seg.limit<  103)
> +		return false;
> +	r = kvm_read_guest_virt_system(tr_seg.base + 102,&io_bitmap_ptr, 2,
> +				       vcpu, NULL);
> +	if (r != X86EMUL_CONTINUE)
> +		return false;
> +	if (io_bitmap_ptr + port/8>= tr_seg.limit)
> +		return false;
>    

Should this be '>'?  limits are generally inclusive of the byte read 
(i.e. they aren't the size of the segment, but the offset of the last byte).

> +	r = kvm_read_guest_virt_system(tr_seg.base + io_bitmap_ptr + port/8,
> +				&perm, 1, vcpu, NULL);
> +	if (r != X86EMUL_CONTINUE)
> +		return false;
> +	if ((perm>>  bit_idx)&  mask)
> +		return false;
> +	return true;
> +}
> +
>    

-- 
error compiling committee.c: too many arguments to function