[3/4] x86emul: drop pointless and add useful default cases

Commit Message

Jan Beulich June 20, 2016, 11:58 a.m. UTC

There's no point in having default cases when all possible values have
respective case statements, or when there's just a "break" statement.

Otoh the two main switch() statements better get default cases added,
just to cover the case of someone altering one of the two lookup arrays
without suitably changing these switch statements.

Signed-off-by: Jan Beulich <[e-mail]>
x86emul: drop pointless and add useful default cases

There's no point in having default cases when all possible values have
respective case statements, or when there's just a "break" statement.

Otoh the two main switch() statements better get default cases added,
just to cover the case of someone altering one of the two lookup arrays
without suitably changing these switch statements.

Signed-off-by: Jan Beulich <[e-mail]>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1379,7 +1379,6 @@ decode_segment(uint8_t modrm_reg)
     case 3: return x86_seg_ds;
     case 4: return x86_seg_fs;
     case 5: return x86_seg_gs;
-    default: break;
     }
     return decode_segment_failed;
 }
@@ -2996,8 +2995,6 @@ x86_emulate(
             case 7: /* fdivr */
                 emulate_fpu_insn_memsrc("fdivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3128,8 +3125,6 @@ x86_emulate(
             case 7: /* fidivr m32i */
                 emulate_fpu_insn_memsrc("fidivrl", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3352,8 +3347,6 @@ x86_emulate(
             case 7: /* fidivr m16i */
                 emulate_fpu_insn_memsrc("fidivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3431,8 +3424,6 @@ x86_emulate(
                 dst.type = OP_MEM;
                 emulate_fpu_insn_memdst("fistpll", dst.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3750,8 +3741,6 @@ x86_emulate(
             }
             break;
         }
-        default:
-            goto cannot_emulate;
         }
         break;
 
@@ -3845,10 +3834,11 @@ x86_emulate(
             goto push;
         case 7:
             generate_exception_if(1, EXC_UD, -1);
-        default:
-            goto cannot_emulate;
         }
         break;
+
+    default:
+        BUG();
     }
 
  writeback:
@@ -4815,6 +4805,9 @@ x86_emulate(
             break;
         }
         break;
+
+    default:
+        BUG();
     }
     goto writeback;

Comments

Andrew Cooper June 23, 2016, 10:44 a.m. UTC | #1

On 20/06/16 12:58, Jan Beulich wrote:
> @@ -3845,10 +3834,11 @@ x86_emulate(
>              goto push;
>          case 7:
>              generate_exception_if(1, EXC_UD, -1);
> -        default:
> -            goto cannot_emulate;
>          }
>          break;
> +
> +    default:
> +        BUG();

These introduce DoS vulnerabilities if there is indeed a path to default.

Could I recommend instead a one-time printk() dumping the instruction
stream which lead to here, an ASSERT_UNREACHABLE(), and a goto
cannot_emulate?

That way, a production build won't crash.

~Andrew

Jan Beulich June 23, 2016, 12:28 p.m. UTC | #2

>>> On 23.06.16 at 12:44, <[e-mail]> wrote:
> On 20/06/16 12:58, Jan Beulich wrote:
>> @@ -3845,10 +3834,11 @@ x86_emulate(
>>              goto push;
>>          case 7:
>>              generate_exception_if(1, EXC_UD, -1);
>> -        default:
>> -            goto cannot_emulate;
>>          }
>>          break;
>> +
>> +    default:
>> +        BUG();
> 
> These introduce DoS vulnerabilities if there is indeed a path to default.
> 
> Could I recommend instead a one-time printk() dumping the instruction
> stream which lead to here, an ASSERT_UNREACHABLE(), and a goto
> cannot_emulate?
> 
> That way, a production build won't crash.

Good point, will do.

Jan

Patch

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1379,7 +1379,6 @@  decode_segment(uint8_t modrm_reg)
     case 3: return x86_seg_ds;
     case 4: return x86_seg_fs;
     case 5: return x86_seg_gs;
-    default: break;
     }
     return decode_segment_failed;
 }
@@ -2996,8 +2995,6 @@  x86_emulate(
             case 7: /* fdivr */
                 emulate_fpu_insn_memsrc("fdivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3128,8 +3125,6 @@  x86_emulate(
             case 7: /* fidivr m32i */
                 emulate_fpu_insn_memsrc("fidivrl", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3352,8 +3347,6 @@  x86_emulate(
             case 7: /* fidivr m16i */
                 emulate_fpu_insn_memsrc("fidivrs", src.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3431,8 +3424,6 @@  x86_emulate(
                 dst.type = OP_MEM;
                 emulate_fpu_insn_memdst("fistpll", dst.val);
                 break;
-            default:
-                goto cannot_emulate;
             }
         }
         break;
@@ -3750,8 +3741,6 @@  x86_emulate(
             }
             break;
         }
-        default:
-            goto cannot_emulate;
         }
         break;
 
@@ -3845,10 +3834,11 @@  x86_emulate(
             goto push;
         case 7:
             generate_exception_if(1, EXC_UD, -1);
-        default:
-            goto cannot_emulate;
         }
         break;
+
+    default:
+        BUG();
     }
 
  writeback:
@@ -4815,6 +4805,9 @@  x86_emulate(
             break;
         }
         break;
+
+    default:
+        BUG();
     }
     goto writeback;