| Message ID | fb917afb-62d1-42f0-83e6-276cae67569d@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86emul: set (fake) operand size for AVX512CD broadcast insns | expand |
On 21/08/2024 10:28 am, Jan Beulich wrote: > Back at the time I failed to pay attention to op_bytes still being zero > when reaching the respective case block: With the ext0f38_table[] > entries having simd_packed_int, the defaulting at the bottom of > x86emul_decode() won't set the field to non-zero for F3-prefixed insns. > > Fixes: 37ccca740c26 ("x86emul: support AVX512CD insns") > Signed-off-by: Jan Beulich <jbeulich@suse.com> This is the second such patch. Does that mean there should be an assertion somewhere? ~Andrew > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -5929,6 +5929,7 @@ x86_emulate( > evex.w == ((b >> 4) & 1)), > X86_EXC_UD); > d |= TwoOp; > + op_bytes = 1; /* fake */ > /* fall through */ > case X86EMUL_OPC_EVEX_66(0x0f38, 0xc4): /* vpconflict{d,q} [xyz]mm/mem,[xyz]mm{k} */ > fault_suppression = false;
On 21.08.2024 12:35, Andrew Cooper wrote: > On 21/08/2024 10:28 am, Jan Beulich wrote: >> Back at the time I failed to pay attention to op_bytes still being zero >> when reaching the respective case block: With the ext0f38_table[] >> entries having simd_packed_int, the defaulting at the bottom of >> x86emul_decode() won't set the field to non-zero for F3-prefixed insns. >> >> Fixes: 37ccca740c26 ("x86emul: support AVX512CD insns") >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > This is the second such patch. Indeed. The first one was a result of me doing some AVX10.2 work, covering new similar insn forms. That finding then prompted me to do an audit, resulting in this 2nd finding. > Does that mean there should be an assertion somewhere? Not an assertion, as there actually is a check already: else if ( state->simd_size != simd_none ) { generate_exception_if(!op_bytes, X86_EXC_UD); That check is what is causing emulation to fail when op_bytes isn't set ahead of trying to invoke a SIMD insn via the shared logic near the end of the function. I don't think it needs to be any stronger than that. The reason this wasn't noticed so far is merely because no testing we have in place ever exercises these insns. Which is a shortcoming, yes, but one that's not very easy to overcome (as in: if we wanted to, that would likely mean writing quite a bit of new testing code, to cover everything that isn't covered right now). For insns not accessing memory the value actually isn't needed / used. An alternative might therefore be to move that check into the OP_MEM conditional, and drop the fake setting of op_bytes (there are a few more similar to the one being added here). Jan
On 21.08.2024 12:49, Jan Beulich wrote: > On 21.08.2024 12:35, Andrew Cooper wrote: >> On 21/08/2024 10:28 am, Jan Beulich wrote: >>> Back at the time I failed to pay attention to op_bytes still being zero >>> when reaching the respective case block: With the ext0f38_table[] >>> entries having simd_packed_int, the defaulting at the bottom of >>> x86emul_decode() won't set the field to non-zero for F3-prefixed insns. >>> >>> Fixes: 37ccca740c26 ("x86emul: support AVX512CD insns") >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> This is the second such patch. > > Indeed. The first one was a result of me doing some AVX10.2 work, > covering new similar insn forms. That finding then prompted me to do > an audit, resulting in this 2nd finding. > >> Does that mean there should be an assertion somewhere? > > Not an assertion, as there actually is a check already: > > else if ( state->simd_size != simd_none ) > { > generate_exception_if(!op_bytes, X86_EXC_UD); > > That check is what is causing emulation to fail when op_bytes isn't set > ahead of trying to invoke a SIMD insn via the shared logic near the end > of the function. I don't think it needs to be any stronger than that. > The reason this wasn't noticed so far is merely because no testing we > have in place ever exercises these insns. Which is a shortcoming, yes, > but one that's not very easy to overcome (as in: if we wanted to, that > would likely mean writing quite a bit of new testing code, to cover > everything that isn't covered right now). Thinking about it - we have EXPECT(), which is perhaps better to use here. That way, even if not covered by our testing, fuzzers will be in the position to trigger it (on capable hardware only of course). > For insns not accessing memory the value actually isn't needed / used. > An alternative might therefore be to move that check into the OP_MEM > conditional, and drop the fake setting of op_bytes (there are a few > more similar to the one being added here). Question then is whether to also move it, as indicated. From a debugging perspective it's likely better to keep where it is, while from a user perspective allowing reg-only insns to go through (despite the internal error) might be preferable. Jan
On 21/08/2024 10:28 am, Jan Beulich wrote: > Back at the time I failed to pay attention to op_bytes still being zero > when reaching the respective case block: With the ext0f38_table[] > entries having simd_packed_int, the defaulting at the bottom of > x86emul_decode() won't set the field to non-zero for F3-prefixed insns. > > Fixes: 37ccca740c26 ("x86emul: support AVX512CD insns") > Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -5929,6 +5929,7 @@ x86_emulate( evex.w == ((b >> 4) & 1)), X86_EXC_UD); d |= TwoOp; + op_bytes = 1; /* fake */ /* fall through */ case X86EMUL_OPC_EVEX_66(0x0f38, 0xc4): /* vpconflict{d,q} [xyz]mm/mem,[xyz]mm{k} */ fault_suppression = false;
Back at the time I failed to pay attention to op_bytes still being zero when reaching the respective case block: With the ext0f38_table[] entries having simd_packed_int, the defaulting at the bottom of x86emul_decode() won't set the field to non-zero for F3-prefixed insns. Fixes: 37ccca740c26 ("x86emul: support AVX512CD insns") Signed-off-by: Jan Beulich <jbeulich@suse.com>