linux-kernel - Re: [PATCH v14 01/22] KVM: x86: Introduce KVM_{G,S}ET_ONE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aMGyw1B7Cw_xHjh3@google.com>
Date: Wed, 10 Sep 2025 10:17:55 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, acme@...hat.com, 
	bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com, john.allen@....com, 
	mingo@...nel.org, mingo@...hat.com, minipli@...ecurity.net, 
	mlevitsk@...hat.com, namhyung@...nel.org, pbonzini@...hat.com, 
	prsampat@....com, rick.p.edgecombe@...el.com, shuah@...nel.org, 
	tglx@...utronix.de, weijiang.yang@...el.com, x86@...nel.org, xin@...or.com, 
	xiaoyao.li@...el.com
Subject: Re: [PATCH v14 01/22] KVM: x86: Introduce KVM_{G,S}ET_ONE_REG uAPIs support

On Tue, Sep 09, 2025, Chao Gao wrote:
> +static int kvm_set_one_msr(struct kvm_vcpu *vcpu, u32 msr, u64 __user *value)
> +{
> +	u64 val;
> +
> +	if (get_user(val, value))
> +		return -EFAULT;
> +
> +	return do_set_msr(vcpu, msr, &val);

This needs to explicitly return -EINVAL on failure, otherwise KVM will return
semi-arbitrary positive values to userspace.

> +}
> +
>  #ifdef CONFIG_X86_64
>  struct pvclock_clock {
>  	int vclock_mode;
> @@ -4737,6 +4762,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>  	case KVM_CAP_IRQFD_RESAMPLE:
>  	case KVM_CAP_MEMORY_FAULT_INFO:
>  	case KVM_CAP_X86_GUEST_MODE:
> +	case KVM_CAP_ONE_REG:

We should add (partial) support for KVM_GET_REG_LIST, otherwise the ABI for
handling GUEST_SSP is effectively undefined.  And because the ioctl is per-vCPU,
utilizing KVM_GET_REG_LIST gives us the opportunity to avoid the horrors we
created with KVM_GET_MSR_INDEX_LIST, where KVM enumerates MSRs that aren't fully
supported by the vCPU.

If we don't enumerate GUEST_SSP via KVM_GET_REG_LIST, then trying to do
KVM_{G,S}ET_ONE_REG will "unexpectedly" fail if the vCPU doesn't have SHSTK.
By enumerating GUEST_SSP in KVM_GET_REG_LIST _if and only if_ it's fully supported,
we'll have a much more explicit ABI than we do for MSRs.  And if we don't do that,
we'd have to special case MSR_KVM_INTERNAL_GUEST_SSP in kvm_is_advertised_msr().

As for MSRs, that's where "partial" support comes in.  For MSRs, I think the least
awful option is to keep using KVM_GET_MSR_INDEX_LIST for enumerating MSRs, and
document that any MSRs that can be accessed via KVM_{G,S}ET_MSRS can be accessed
via KVM_{G,S}ET_ONE_REG.  That avoids having to bake in different behavior for
MSR vs. ONE_REG accesses (and avoids having to add a pile of code to precisely
enumerate support for per-vCPU MSRs).

>  		r = 1;
>  		break;
>  	case KVM_CAP_PRE_FAULT_MEMORY:
> @@ -5915,6 +5941,20 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  	}
>  }
>  
> +struct kvm_x86_reg_id {
> +	__u32 index;
> +	__u8  type;
> +	__u8  rsvd1;
> +	__u8  rsvd2:4;
> +	__u8  size:4;
> +	__u8  x86;
> +};
> +
> +static int kvm_translate_kvm_reg(struct kvm_x86_reg_id *reg)
> +{
> +	return -EINVAL;
> +}
> +
>  long kvm_arch_vcpu_ioctl(struct file *filp,
>  			 unsigned int ioctl, unsigned long arg)
>  {
> @@ -6031,6 +6071,44 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  		srcu_read_unlock(&vcpu->kvm->srcu, idx);
>  		break;
>  	}
> +	case KVM_GET_ONE_REG:
> +	case KVM_SET_ONE_REG: {
> +		struct kvm_x86_reg_id *id;
> +		struct kvm_one_reg reg;
> +		u64 __user *value;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&reg, argp, sizeof(reg)))
> +			break;
> +
> +		r = -EINVAL;
> +		if ((reg.id & KVM_REG_ARCH_MASK) != KVM_REG_X86)
> +			break;
> +
> +		id = (struct kvm_x86_reg_id *)&reg.id;
> +		if (id->rsvd1 || id->rsvd2)
> +			break;
> +
> +		if (id->type == KVM_X86_REG_TYPE_KVM) {
> +			r = kvm_translate_kvm_reg(id);
> +			if (r)
> +				break;
> +		}
> +
> +		r = -EINVAL;
> +		if (id->type != KVM_X86_REG_TYPE_MSR)
> +			break;
> +
> +		if ((reg.id & KVM_REG_SIZE_MASK) != KVM_REG_SIZE_U64)
> +			break;
> +
> +		value = u64_to_user_ptr(reg.addr);
> +		if (ioctl == KVM_GET_ONE_REG)
> +			r = kvm_get_one_msr(vcpu, id->index, value);
> +		else
> +			r = kvm_set_one_msr(vcpu, id->index, value);
> +		break;
> +	}

I think it makes sense to put this in a separate helper, if only so that the
error returns are more obvious.

>  	case KVM_TPR_ACCESS_REPORTING: {
>  		struct kvm_tpr_access_ctl tac;
>  
> -- 
> 2.47.3
>