1. Patchew
  2. Xen
  3. View series

[PATCH] x86emul: V{,P}{COMPRESS,EXPAND}* can (wrongly) trigger assertion

Jan Beulich posted 1 patch 1 week ago
Failed in applying to current master (apply log)
[PATCH] x86emul: V{,P}{COMPRESS,EXPAND}* can (wrongly) trigger assertion
Posted by Jan Beulich 1 week ago 
AFL has pointed out that the op_bytes-is-not-0 assertion in common SIMD
handling can trigger for these insns. Indeed when the (relevant part of)
the controlling mask register is 0, no memory is accessed at all. Leave
op_bytes unaltered in this case, to engage the short-circuiting in common
SIMD handling when fault_suppression is true and op_bytes is 0.

While there also correct a related typo in the test harness.

Fixes: 65f82d4ce1ea ("x86emul: support AVX512{F,_VBMI2} compress/expand insns")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
In a release build, due to op_mask being 0 when n is 0, the short-circuit
mentioned will prevent any damage.

--- a/tools/tests/x86_emulator/predicates.c
+++ b/tools/tests/x86_emulator/predicates.c
@@ -1946,7 +1946,7 @@ static const struct evex {
     { { 0x83 }, 2, T, R, pfx_66, W1, Ln }, /* vpmultishiftqb */
     { { 0x88 }, 2, T, R, pfx_66, Wn, Ln }, /* vpexpandp{s,d} */
     { { 0x89 }, 2, T, R, pfx_66, Wn, Ln }, /* vpexpand{d,q} */
-    { { 0x8a }, 2, T, W, pfx_66, Wn, Ln }, /* vpcompressp{s,d} */
+    { { 0x8a }, 2, T, W, pfx_66, Wn, Ln }, /* vcompressp{s,d} */
     { { 0x8b }, 2, T, W, pfx_66, Wn, Ln }, /* vpcompress{d,q} */
     { { 0x8d }, 2, F, R, pfx_66, Wn, Ln }, /* vperm{b,w} */
     { { 0x8f }, 2, F, R, pfx_66, W0, Ln }, /* vpshufbitqmb */
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -6236,9 +6236,11 @@ x86_emulate(
             ASSERT(op_bytes == n * elem_bytes);
             op_mask &= ~0ULL >> (64 - n);
             n = hweight64(op_mask);
-            op_bytes = n * elem_bytes;
             if ( n )
+            {
+                op_bytes = n * elem_bytes;
                 op_mask = ~0ULL >> (64 - n);
+            }
         }
         goto simd_zmm;
Re: [PATCH] x86emul: V{,P}{COMPRESS,EXPAND}* can (wrongly) trigger assertion
Posted by Teddy Astie 1 week ago 
Le 16/07/2026 à 12:45, Jan Beulich a écrit :
> AFL has pointed out that the op_bytes-is-not-0 assertion in common SIMD
> handling can trigger for these insns. Indeed when the (relevant part of)
> the controlling mask register is 0, no memory is accessed at all. Leave
> op_bytes unaltered in this case, to engage the short-circuiting in common
> SIMD handling when fault_suppression is true and op_bytes is 0.
> 
> While there also correct a related typo in the test harness.
> 
> Fixes: 65f82d4ce1ea ("x86emul: support AVX512{F,_VBMI2} compress/expand insns")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> In a release build, due to op_mask being 0 when n is 0, the short-circuit
> mentioned will prevent any damage.
> 
> --- a/tools/tests/x86_emulator/predicates.c
> +++ b/tools/tests/x86_emulator/predicates.c
> @@ -1946,7 +1946,7 @@ static const struct evex {
>       { { 0x83 }, 2, T, R, pfx_66, W1, Ln }, /* vpmultishiftqb */
>       { { 0x88 }, 2, T, R, pfx_66, Wn, Ln }, /* vpexpandp{s,d} */
>       { { 0x89 }, 2, T, R, pfx_66, Wn, Ln }, /* vpexpand{d,q} */
> -    { { 0x8a }, 2, T, W, pfx_66, Wn, Ln }, /* vpcompressp{s,d} */
> +    { { 0x8a }, 2, T, W, pfx_66, Wn, Ln }, /* vcompressp{s,d} */
>       { { 0x8b }, 2, T, W, pfx_66, Wn, Ln }, /* vpcompress{d,q} */
>       { { 0x8d }, 2, F, R, pfx_66, Wn, Ln }, /* vperm{b,w} */
>       { { 0x8f }, 2, F, R, pfx_66, W0, Ln }, /* vpshufbitqmb */
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -6236,9 +6236,11 @@ x86_emulate(
>               ASSERT(op_bytes == n * elem_bytes);
>               op_mask &= ~0ULL >> (64 - n);
>               n = hweight64(op_mask);
> -            op_bytes = n * elem_bytes;
>               if ( n )
> +            {
> +                op_bytes = n * elem_bytes;
>                   op_mask = ~0ULL >> (64 - n);
> +            }
>           }
>           goto simd_zmm;
>   
> 

That looks like a bit of a hack in my understanding.
Is there anything specific preventing the common SIMD logic from 
accepting op_bytes being 0 ? Otherwise, we risk seeing similar issues 
with other instructions (future or current).
Re: [PATCH] x86emul: V{,P}{COMPRESS,EXPAND}* can (wrongly) trigger assertion
Posted by Jan Beulich 1 week ago 
On 16.07.2026 15:35, Teddy Astie wrote:
> Le 16/07/2026 à 12:45, Jan Beulich a écrit :
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -6236,9 +6236,11 @@ x86_emulate(
>>               ASSERT(op_bytes == n * elem_bytes);
>>               op_mask &= ~0ULL >> (64 - n);
>>               n = hweight64(op_mask);
>> -            op_bytes = n * elem_bytes;
>>               if ( n )
>> +            {
>> +                op_bytes = n * elem_bytes;
>>                   op_mask = ~0ULL >> (64 - n);
>> +            }
>>           }
>>           goto simd_zmm;
>>   
>>
> 
> That looks like a bit of a hack in my understanding.

It may be looked at it this way, yes. The entire file may be looked at this way,
really.

> Is there anything specific preventing the common SIMD logic from 
> accepting op_bytes being 0 ? Otherwise, we risk seeing similar issues 
> with other instructions (future or current).

Requiring op_bytes to be non-zero is a safe-guard. If any of the cases engaging
common SIMD emulation would fail to set this up correctly, it would be noticed
only if the path was actually taken (to serve a VM, or in the test or fuzzing
harnesses) and if results were checked to be correct. The test harness can't
reasonably guarantee all paths to be taken. Whereas the running fuzzing harness
doesn't prove that execution was actually correct; it only proves that we
didn't crash or hang. Skipping a memory access where one would be needed would
likely go entirely unnoticed there.

Jan
Re: [PATCH] x86emul: V{,P}{COMPRESS,EXPAND}* can (wrongly) trigger assertion
Posted by Andrew Cooper 1 week ago 
On 16/07/2026 11:43 am, Jan Beulich wrote:
> AFL has pointed out that the op_bytes-is-not-0 assertion in common SIMD
> handling can trigger for these insns. Indeed when the (relevant part of)
> the controlling mask register is 0, no memory is accessed at all. Leave
> op_bytes unaltered in this case, to engage the short-circuiting in common
> SIMD handling when fault_suppression is true and op_bytes is 0.
>
> While there also correct a related typo in the test harness.
>
> Fixes: 65f82d4ce1ea ("x86emul: support AVX512{F,_VBMI2} compress/expand insns")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Is this local AFL testing?  I'm slightly surprised that OSS-Fuzz hasn't
found this.
Re: [PATCH] x86emul: V{,P}{COMPRESS,EXPAND}* can (wrongly) trigger assertion
Posted by Jan Beulich 1 week ago 
On 16.07.2026 12:44, Andrew Cooper wrote:
> On 16/07/2026 11:43 am, Jan Beulich wrote:
>> AFL has pointed out that the op_bytes-is-not-0 assertion in common SIMD
>> handling can trigger for these insns. Indeed when the (relevant part of)
>> the controlling mask register is 0, no memory is accessed at all. Leave
>> op_bytes unaltered in this case, to engage the short-circuiting in common
>> SIMD handling when fault_suppression is true and op_bytes is 0.
>>
>> While there also correct a related typo in the test harness.
>>
>> Fixes: 65f82d4ce1ea ("x86emul: support AVX512{F,_VBMI2} compress/expand insns")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> Is this local AFL testing?  I'm slightly surprised that OSS-Fuzz hasn't
> found this.

Yes, I'm trying to remember to run the fuzzer every once in a while. Earlier
runs of mine didn't spot the issue either. (I'm presently re-running things,
to have a limited level of certainty that the issues it found are all sorted
now. The other issues it found were in patches I continue to have pending.)

Jan
Re: [PATCH] x86emul: V{,P}{COMPRESS,EXPAND}* can (wrongly) trigger assertion
Posted by Andrew Cooper 1 week ago 
On 16/07/2026 11:50 am, Jan Beulich wrote:
> On 16.07.2026 12:44, Andrew Cooper wrote:
>> On 16/07/2026 11:43 am, Jan Beulich wrote:
>>> AFL has pointed out that the op_bytes-is-not-0 assertion in common SIMD
>>> handling can trigger for these insns. Indeed when the (relevant part of)
>>> the controlling mask register is 0, no memory is accessed at all. Leave
>>> op_bytes unaltered in this case, to engage the short-circuiting in common
>>> SIMD handling when fault_suppression is true and op_bytes is 0.
>>>
>>> While there also correct a related typo in the test harness.
>>>
>>> Fixes: 65f82d4ce1ea ("x86emul: support AVX512{F,_VBMI2} compress/expand insns")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks.
>
>> Is this local AFL testing?  I'm slightly surprised that OSS-Fuzz hasn't
>> found this.
> Yes, I'm trying to remember to run the fuzzer every once in a while. Earlier
> runs of mine didn't spot the issue either. (I'm presently re-running things,
> to have a limited level of certainty that the issues it found are all sorted
> now. The other issues it found were in patches I continue to have pending.)

When I was doing AFL testing a while back, there was an ancillary
project which would take the fuzzing corpus (as it was being generated),
run it with a GCOV-enabled binary and produce an aggregate LCOV report
in real-time.

I did once get to 100% coverage, but it took AFL an awful long time to
find the final basic block, which was somewhere in the signed divide
path if memory serves.

If we want to get fancy, instead of using a single byte input as the
seed corpus, we could write out several which tickle the rarest paths. 
That ought to shorten the total time substantially.

~Andrew

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.