diff mbox series

x86emul: convert op_bytes/opc checks in SIMD emulation

Message ID bfc9722a-cb6d-45d0-9351-ddfcd0bbb2e0@suse.com (mailing list archive)
State New
Headers show
Series x86emul: convert op_bytes/opc checks in SIMD emulation | expand

Commit Message

Jan Beulich Aug. 21, 2024, 1:30 p.m. UTC 
Delivering #UD for an internal shortcoming of the emulator isn't quite
right. Similarly BUG() is bigger a hammer than needed.

Switch to using EXPECT() instead.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Andrew Cooper Aug. 21, 2024, 1:37 p.m. UTC | #1 
On 21/08/2024 2:30 pm, Jan Beulich wrote:
> Delivering #UD for an internal shortcoming of the emulator isn't quite
> right. Similarly BUG() is bigger a hammer than needed.
>
> Switch to using EXPECT() instead.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

To confirm, this is ASSERT_UNREACHABLE() (which fuzzing will now notice
as an error), and unhandleable in release builds (which ultimately ends
up in #UD)?

I think it would be helpful to at least note the fuzzing aspect in the
commit message.

>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -8114,13 +8114,13 @@ x86_emulate(
>      }
>      else if ( state->simd_size != simd_none )
>      {
> -        generate_exception_if(!op_bytes, X86_EXC_UD);
>          generate_exception_if((vex.opcx && (d & TwoOp) &&
>                                 (vex.reg != 0xf || (evex_encoded() && !evex.RX))),
>                                X86_EXC_UD);
>  
> -        if ( !opc )
> -            BUG();
> +        EXPECT(op_bytes);
> +        EXPECT(opc);

This is the only BUG() in x86_emulate.c, and it's right to get rid of it
IMO.

Therefore, we should have a hunk removing it from
tools/tests/x86_emulator/x86-emulate.h too, which will prevent
reintroduction.

Maybe even undef BUG somewhere in x86_emulate/private.h?

~Andrew
Jan Beulich Aug. 21, 2024, 3:03 p.m. UTC | #2 
On 21.08.2024 15:37, Andrew Cooper wrote:
> On 21/08/2024 2:30 pm, Jan Beulich wrote:
>> Delivering #UD for an internal shortcoming of the emulator isn't quite
>> right. Similarly BUG() is bigger a hammer than needed.
>>
>> Switch to using EXPECT() instead.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> To confirm, this is ASSERT_UNREACHABLE() (which fuzzing will now notice
> as an error), and unhandleable in release builds (which ultimately ends
> up in #UD)?

Yes, at least mostly. What exactly UNHANDLEABLE converts to depends on
the use site, I think.

> I think it would be helpful to at least note the fuzzing aspect in the
> commit message.

I've added "This way even for insns not covered by the test harness
fuzzers will have a chance of noticing issues, should any still exist
or new ones be introduced" to the 2nd paragraph.

>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -8114,13 +8114,13 @@ x86_emulate(
>>      }
>>      else if ( state->simd_size != simd_none )
>>      {
>> -        generate_exception_if(!op_bytes, X86_EXC_UD);
>>          generate_exception_if((vex.opcx && (d & TwoOp) &&
>>                                 (vex.reg != 0xf || (evex_encoded() && !evex.RX))),
>>                                X86_EXC_UD);
>>  
>> -        if ( !opc )
>> -            BUG();
>> +        EXPECT(op_bytes);
>> +        EXPECT(opc);
> 
> This is the only BUG() in x86_emulate.c, and it's right to get rid of it
> IMO.
> 
> Therefore, we should have a hunk removing it from
> tools/tests/x86_emulator/x86-emulate.h too, which will prevent
> reintroduction.
> 
> Maybe even undef BUG somewhere in x86_emulate/private.h?

Both of these actions can only be taken if the other BUG() in decode.c
also goes away. But yes, what you suggest is probably the best course of
action. I guess I'll do that in yet another patch, though.

Jan
Andrew Cooper Aug. 21, 2024, 3:10 p.m. UTC | #3 
On 21/08/2024 4:03 pm, Jan Beulich wrote:
> On 21.08.2024 15:37, Andrew Cooper wrote:
>> On 21/08/2024 2:30 pm, Jan Beulich wrote:
>>> Delivering #UD for an internal shortcoming of the emulator isn't quite
>>> right. Similarly BUG() is bigger a hammer than needed.
>>>
>>> Switch to using EXPECT() instead.
>>>
>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> To confirm, this is ASSERT_UNREACHABLE() (which fuzzing will now notice
>> as an error), and unhandleable in release builds (which ultimately ends
>> up in #UD)?
> Yes, at least mostly. What exactly UNHANDLEABLE converts to depends on
> the use site, I think.

Sure, but it's a well defined non-fatal exit path.

>
>> I think it would be helpful to at least note the fuzzing aspect in the
>> commit message.
> I've added "This way even for insns not covered by the test harness
> fuzzers will have a chance of noticing issues, should any still exist
> or new ones be introduced" to the 2nd paragraph.

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
>>> @@ -8114,13 +8114,13 @@ x86_emulate(
>>>      }
>>>      else if ( state->simd_size != simd_none )
>>>      {
>>> -        generate_exception_if(!op_bytes, X86_EXC_UD);
>>>          generate_exception_if((vex.opcx && (d & TwoOp) &&
>>>                                 (vex.reg != 0xf || (evex_encoded() && !evex.RX))),
>>>                                X86_EXC_UD);
>>>  
>>> -        if ( !opc )
>>> -            BUG();
>>> +        EXPECT(op_bytes);
>>> +        EXPECT(opc);
>> This is the only BUG() in x86_emulate.c, and it's right to get rid of it
>> IMO.
>>
>> Therefore, we should have a hunk removing it from
>> tools/tests/x86_emulator/x86-emulate.h too, which will prevent
>> reintroduction.
>>
>> Maybe even undef BUG somewhere in x86_emulate/private.h?
> Both of these actions can only be taken if the other BUG() in decode.c
> also goes away. But yes, what you suggest is probably the best course of
> action. I guess I'll do that in yet another patch, though.

Is that BUG() local to your tree?  I cant see it in staging.

Deferring this to a later patch is fine.

~Andrew
Jan Beulich Aug. 21, 2024, 3:56 p.m. UTC | #4 
On 21.08.2024 17:10, Andrew Cooper wrote:
> On 21/08/2024 4:03 pm, Jan Beulich wrote:
>> On 21.08.2024 15:37, Andrew Cooper wrote:
>>> I think it would be helpful to at least note the fuzzing aspect in the
>>> commit message.
>> I've added "This way even for insns not covered by the test harness
>> fuzzers will have a chance of noticing issues, should any still exist
>> or new ones be introduced" to the 2nd paragraph.
> 
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> @@ -8114,13 +8114,13 @@ x86_emulate(
>>>>      }
>>>>      else if ( state->simd_size != simd_none )
>>>>      {
>>>> -        generate_exception_if(!op_bytes, X86_EXC_UD);
>>>>          generate_exception_if((vex.opcx && (d & TwoOp) &&
>>>>                                 (vex.reg != 0xf || (evex_encoded() && !evex.RX))),
>>>>                                X86_EXC_UD);
>>>>  
>>>> -        if ( !opc )
>>>> -            BUG();
>>>> +        EXPECT(op_bytes);
>>>> +        EXPECT(opc);
>>> This is the only BUG() in x86_emulate.c, and it's right to get rid of it
>>> IMO.
>>>
>>> Therefore, we should have a hunk removing it from
>>> tools/tests/x86_emulator/x86-emulate.h too, which will prevent
>>> reintroduction.
>>>
>>> Maybe even undef BUG somewhere in x86_emulate/private.h?
>> Both of these actions can only be taken if the other BUG() in decode.c
>> also goes away. But yes, what you suggest is probably the best course of
>> action. I guess I'll do that in yet another patch, though.
> 
> Is that BUG() local to your tree?  I cant see it in staging.

I first thought it would be when you mentioned you found only one, but it's
been there for a long time[1], in VEX/EVEX prefix decoding. With a comment
added by you[2].

Jan

[1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=11c35f84b53622c429071049b830bb9b7a880eff
[2] https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=933acddf74213f77a5adbb2c10ccd3f72634d456
Andrew Cooper Aug. 21, 2024, 4:24 p.m. UTC | #5 
On 21/08/2024 4:56 pm, Jan Beulich wrote:
>>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>>> @@ -8114,13 +8114,13 @@ x86_emulate(
>>>>>      }
>>>>>      else if ( state->simd_size != simd_none )
>>>>>      {
>>>>> -        generate_exception_if(!op_bytes, X86_EXC_UD);
>>>>>          generate_exception_if((vex.opcx && (d & TwoOp) &&
>>>>>                                 (vex.reg != 0xf || (evex_encoded() && !evex.RX))),
>>>>>                                X86_EXC_UD);
>>>>>  
>>>>> -        if ( !opc )
>>>>> -            BUG();
>>>>> +        EXPECT(op_bytes);
>>>>> +        EXPECT(opc);
>>>> This is the only BUG() in x86_emulate.c, and it's right to get rid of it
>>>> IMO.
>>>>
>>>> Therefore, we should have a hunk removing it from
>>>> tools/tests/x86_emulator/x86-emulate.h too, which will prevent
>>>> reintroduction.
>>>>
>>>> Maybe even undef BUG somewhere in x86_emulate/private.h?
>>> Both of these actions can only be taken if the other BUG() in decode.c
>>> also goes away. But yes, what you suggest is probably the best course of
>>> action. I guess I'll do that in yet another patch, though.
>> Is that BUG() local to your tree?  I cant see it in staging.
> I first thought it would be when you mentioned you found only one, but it's
> been there for a long time[1], in VEX/EVEX prefix decoding. With a comment
> added by you[2].

Oh, I'm clearly blind.

But yes, that one wants to become EXPECT() too, I'd say.

~Andrew
diff mbox series

Patch

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -8114,13 +8114,13 @@  x86_emulate(
     }
     else if ( state->simd_size != simd_none )
     {
-        generate_exception_if(!op_bytes, X86_EXC_UD);
         generate_exception_if((vex.opcx && (d & TwoOp) &&
                                (vex.reg != 0xf || (evex_encoded() && !evex.RX))),
                               X86_EXC_UD);
 
-        if ( !opc )
-            BUG();
+        EXPECT(op_bytes);
+        EXPECT(opc);
+
         if ( evex_encoded() )
         {
             opc[insn_bytes - EVEX_PFX_BYTES] = 0xc3;