[PATCH 2/5] i386/hvf: Fix for UB in handling CPUID function 0xD

Phil Dennis-Jordan posted 5 patches 2 weeks, 4 days ago
[PATCH 2/5] i386/hvf: Fix for UB in handling CPUID function 0xD
Posted by Phil Dennis-Jordan 2 weeks, 4 days ago
The handling for CPUID function 0xD (supported XSAVE features) was
improved in a recent patch. Unfortunately, this appears to have
introduced undefined behaviour for cases where ecx > 30, as the result
of (1 << idx) is undefined if idx > 30.

Per Intel SDM section 13.2, the behaviour for ecx values up to and
including 62 are specified. This change therefore specifically sets
all registers returned by the CPUID instruction to 0 for 63 and higher.
Furthermore, the bit shift uses uint64_t, where behaviour for the entire
range of 2..62 is safe and correct.

Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 target/i386/hvf/x86_cpuid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/hvf/x86_cpuid.c b/target/i386/hvf/x86_cpuid.c
index ac922d7fd16..9d9ccaa815d 100644
--- a/target/i386/hvf/x86_cpuid.c
+++ b/target/i386/hvf/x86_cpuid.c
@@ -119,8 +119,8 @@ uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx,
         eax = 0;
         break;
     case 0xD:
-        if (!supported_xcr0 ||
-            (idx > 1 && !(supported_xcr0 & (1 << idx)))) {
+        if (!supported_xcr0 || idx >= 63 ||
+            (idx > 1 && !(supported_xcr0 & (UINT64_C(1) << idx)))) {
             eax = ebx = ecx = edx = 0;
             break;
         }
-- 
2.39.3 (Apple Git-145)
Re: [PATCH 2/5] i386/hvf: Fix for UB in handling CPUID function 0xD
Posted by Roman Bolshakov 2 weeks, 3 days ago
On Tue, Nov 05, 2024 at 04:57:57PM +0100, Phil Dennis-Jordan wrote:
> The handling for CPUID function 0xD (supported XSAVE features) was
> improved in a recent patch. Unfortunately, this appears to have
> introduced undefined behaviour for cases where ecx > 30, as the result
> of (1 << idx) is undefined if idx > 30.
> 
> Per Intel SDM section 13.2, the behaviour for ecx values up to and
> including 62 are specified. This change therefore specifically sets
> all registers returned by the CPUID instruction to 0 for 63 and higher.
> Furthermore, the bit shift uses uint64_t, where behaviour for the entire
> range of 2..62 is safe and correct.
> 

Thanks for correcting the regression.

Reviewed-by: Roman Bolshakov <rbolshakov@ddn.com>