From: Peter Maydell <peter.maydell@linaro.org> To: Eduardo Habkost <ehabkost@redhat.com> Cc: QEMU Developers <qemu-devel@nongnu.org>, Paolo Bonzini <pbonzini@redhat.com>, kvm-devel <kvm@vger.kernel.org>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Vitaly Kuznetsov <vkuznets@redhat.com> Subject: Re: [PULL 04/11] i386: expand Hyper-V features during CPU feature expansion time Date: Thu, 15 Jul 2021 21:51:58 +0100 [thread overview] Message-ID: <CAFEAcA-nif_Z0guHx4q4NUg=FEyhUz8kkAvfZ58916yp6TXT7Q@mail.gmail.com> (raw) In-Reply-To: <20210713160957.3269017-5-ehabkost@redhat.com> On Tue, 13 Jul 2021 at 17:19, Eduardo Habkost <ehabkost@redhat.com> wrote: > > From: Vitaly Kuznetsov <vkuznets@redhat.com> > > To make Hyper-V features appear in e.g. QMP query-cpu-model-expansion we > need to expand and set the corresponding CPUID leaves early. Modify > x86_cpu_get_supported_feature_word() to call newly intoduced Hyper-V > specific kvm_hv_get_supported_cpuid() instead of > kvm_arch_get_supported_cpuid(). We can't use kvm_arch_get_supported_cpuid() > as Hyper-V specific CPUID leaves intersect with KVM's. > > Note, early expansion will only happen when KVM supports system wide > KVM_GET_SUPPORTED_HV_CPUID ioctl (KVM_CAP_SYS_HYPERV_CPUID). > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Message-Id: <20210608120817.1325125-6-vkuznets@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Hi; Coverity reports an issue in this code (CID 1458243): > -static bool hyperv_expand_features(CPUState *cs, Error **errp) > +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp) > { > - X86CPU *cpu = X86_CPU(cs); > + CPUState *cs = CPU(cpu); > > if (!hyperv_enabled(cpu)) > return true; > > + /* > + * When kvm_hyperv_expand_features is called at CPU feature expansion > + * time per-CPU kvm_state is not available yet so we can only proceed > + * when KVM_CAP_SYS_HYPERV_CPUID is supported. > + */ > + if (!cs->kvm_state && > + !kvm_check_extension(kvm_state, KVM_CAP_SYS_HYPERV_CPUID)) > + return true; Here we check whether cs->kvm_state is NULL, but even if it is NULL we can still continue execution further through the function. Later in the function we call hv_cpuid_get_host(), which in turn can call get_supported_hv_cpuid_legacy(), which can dereference cs->kvm_state without checking it. So either the check on cs->kvm_state above is unnecessary, or we need to handle it being NULL in some way other than falling through. Side note: this change isn't in line with our coding style, which requires braces around the body of the if(). thanks -- PMM
WARNING: multiple messages have this Message-ID (diff)
From: Peter Maydell <peter.maydell@linaro.org> To: Eduardo Habkost <ehabkost@redhat.com> Cc: kvm-devel <kvm@vger.kernel.org>, "Michael S. Tsirkin" <mst@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, QEMU Developers <qemu-devel@nongnu.org>, Paolo Bonzini <pbonzini@redhat.com>, Vitaly Kuznetsov <vkuznets@redhat.com> Subject: Re: [PULL 04/11] i386: expand Hyper-V features during CPU feature expansion time Date: Thu, 15 Jul 2021 21:51:58 +0100 [thread overview] Message-ID: <CAFEAcA-nif_Z0guHx4q4NUg=FEyhUz8kkAvfZ58916yp6TXT7Q@mail.gmail.com> (raw) In-Reply-To: <20210713160957.3269017-5-ehabkost@redhat.com> On Tue, 13 Jul 2021 at 17:19, Eduardo Habkost <ehabkost@redhat.com> wrote: > > From: Vitaly Kuznetsov <vkuznets@redhat.com> > > To make Hyper-V features appear in e.g. QMP query-cpu-model-expansion we > need to expand and set the corresponding CPUID leaves early. Modify > x86_cpu_get_supported_feature_word() to call newly intoduced Hyper-V > specific kvm_hv_get_supported_cpuid() instead of > kvm_arch_get_supported_cpuid(). We can't use kvm_arch_get_supported_cpuid() > as Hyper-V specific CPUID leaves intersect with KVM's. > > Note, early expansion will only happen when KVM supports system wide > KVM_GET_SUPPORTED_HV_CPUID ioctl (KVM_CAP_SYS_HYPERV_CPUID). > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Message-Id: <20210608120817.1325125-6-vkuznets@redhat.com> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> Hi; Coverity reports an issue in this code (CID 1458243): > -static bool hyperv_expand_features(CPUState *cs, Error **errp) > +bool kvm_hyperv_expand_features(X86CPU *cpu, Error **errp) > { > - X86CPU *cpu = X86_CPU(cs); > + CPUState *cs = CPU(cpu); > > if (!hyperv_enabled(cpu)) > return true; > > + /* > + * When kvm_hyperv_expand_features is called at CPU feature expansion > + * time per-CPU kvm_state is not available yet so we can only proceed > + * when KVM_CAP_SYS_HYPERV_CPUID is supported. > + */ > + if (!cs->kvm_state && > + !kvm_check_extension(kvm_state, KVM_CAP_SYS_HYPERV_CPUID)) > + return true; Here we check whether cs->kvm_state is NULL, but even if it is NULL we can still continue execution further through the function. Later in the function we call hv_cpuid_get_host(), which in turn can call get_supported_hv_cpuid_legacy(), which can dereference cs->kvm_state without checking it. So either the check on cs->kvm_state above is unnecessary, or we need to handle it being NULL in some way other than falling through. Side note: this change isn't in line with our coding style, which requires braces around the body of the if(). thanks -- PMM
next prev parent reply other threads:[~2021-07-15 20:52 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-13 16:09 [PULL 00/11] x86 queue, 2021-07-13 Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 01/11] i386: clarify 'hv-passthrough' behavior Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 02/11] i386: hardcode supported eVMCS version to '1' Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 03/11] i386: make hyperv_expand_features() return bool Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 04/11] i386: expand Hyper-V features during CPU feature expansion time Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-15 20:51 ` Peter Maydell [this message] 2021-07-15 20:51 ` Peter Maydell 2021-07-16 9:07 ` Vitaly Kuznetsov 2021-07-16 9:07 ` Vitaly Kuznetsov 2021-07-13 16:09 ` [PULL 05/11] i386: kill off hv_cpuid_check_and_set() Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 06/11] i386: HV_HYPERCALL_AVAILABLE privilege bit is always needed Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 07/11] i386: Hyper-V SynIC requires POST_MESSAGES/SIGNAL_EVENTS privileges Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 08/11] target/i386: suppress CPUID leaves not defined by the CPU vendor Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 09/11] target/i386: Fix cpuid level for AMD Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 10/11] numa: Report expected initiator Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-13 16:09 ` [PULL 11/11] numa: Parse initiator= attribute before cpus= attribute Eduardo Habkost 2021-07-13 16:09 ` Eduardo Habkost 2021-07-14 13:11 ` [PULL 00/11] x86 queue, 2021-07-13 Peter Maydell 2021-07-14 13:11 ` Peter Maydell
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAFEAcA-nif_Z0guHx4q4NUg=FEyhUz8kkAvfZ58916yp6TXT7Q@mail.gmail.com' \ --to=peter.maydell@linaro.org \ --cc=ehabkost@redhat.com \ --cc=kvm@vger.kernel.org \ --cc=marcel.apfelbaum@gmail.com \ --cc=mst@redhat.com \ --cc=mtosatti@redhat.com \ --cc=pbonzini@redhat.com \ --cc=qemu-devel@nongnu.org \ --cc=richard.henderson@linaro.org \ --cc=vkuznets@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.