linux-kernel - Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU has been created

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: <02ee140496680e9e3d364dc13f98d472f3b4d028.camel@intel.com>
Date: Fri, 11 Jul 2025 02:46:11 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "Gao, Chao" <chao.gao@...el.com>
CC: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "bp@...en8.de" <bp@...en8.de>, "Li, Xiaoyao"
	<xiaoyao.li@...el.com>, "nikunj@....com" <nikunj@....com>,
	"thomas.lendacky@....com" <thomas.lendacky@....com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"Yamahata, Isaku" <isaku.yamahata@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] KVM: x86: Reject KVM_SET_TSC_KHZ VM ioctl when vCPU
 has been created

On Fri, 2025-07-11 at 10:17 +0800, Chao Gao wrote:
> > AFAICT the actual updating of kvm->arch.default_tsc_khz needs to be in the
> > kvm->lock mutex too.
> 
> Yep.
> 
> > Please let me know if you found any issue?
> > 
> > diff --git a/Documentation/virt/kvm/api.rst
> > b/Documentation/virt/kvm/api.rst
> > index 43ed57e048a8..86ea1e2b2737 100644
> > --- a/Documentation/virt/kvm/api.rst
> > +++ b/Documentation/virt/kvm/api.rst
> > @@ -2006,7 +2006,7 @@ frequency is KHz.
> > 
> > If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
> > be used as a vm ioctl to set the initial tsc frequency of subsequently
> > -created vCPUs.
> > +created vCPUs.  It must be called before any vCPU is created.
> 
> 		^^ remove one space here.

OK.

> 
> "must be" sounds like a mandatory action, but IIUC the vm ioctl is optional for
> non-CC VMs. I'm not sure if this is just a problem of my interpretation.

The context of that paragraph has "can also be used ...", so to me it's
implied that "if it is called", i.e., it's implied that it is optional for
non-CC VMs.

> 
> To make the API documentation super clear, how about:
> 
> If the KVM_CAP_VM_TSC_CONTROL capability is advertised, this can also
> be used as a vm ioctl to set the initial tsc frequency of vCPUs before
> any vCPU is created. Attempting to call this vm ioctl after vCPU creation
> will return an EINVAL error.

I am not sure we need to mention -EINVAL.  This IOCTL is already returning
-EINVAL for other errors (when invalid user_tsc_khz is supplied).

IMHO: Userspace should just care about whether success or not.  Being
explicit is good, but sometimes it's better to have some room here.

But I'll let Sean/Paolo to decide.

> 
> > 
> > 4.56 KVM_GET_TSC_KHZ
> > --------------------
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2806f7104295..4051c0cacb92 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -7199,9 +7199,12 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned
> > int ioctl, unsigned long arg)
> >                if (user_tsc_khz == 0)
> >                        user_tsc_khz = tsc_khz;
> > 
> > -               WRITE_ONCE(kvm->arch.default_tsc_khz, user_tsc_khz);
> > -               r = 0;
> > -
> > +               mutex_lock(&kvm->lock);
> > +               if (!kvm->created_vcpus) {
> > +                       WRITE_ONCE(kvm->arch.default_tsc_khz,
> > user_tsc_khz);
> > +                       r = 0;
> > +               }
> > +               mutex_unlock(&kvm->lock);
> 
> LGTM.
> 
> >                goto out;
> >        }
> >        case KVM_GET_TSC_KHZ: {