x86emul: fix UB multiplications in S/G handling

The conversion of the shifts to multiplications by the commits tagged
below still wasn't quite right: The multiplications (of signed values)
can overflow, too. As of 298556c7b5f8 ("x86emul: correct 32-bit address
handling for AVX2 gathers") signed multiplication wasn't necessary
anymore, though: The necessary sign-extension (if any) will happen as
well when using intermediate variables of unsigned long types, and
excess address bits are chopped off by truncate_ea().

Fixes: b6a907f8c83d ("x86emul: replace UB shifts")
Fixes: 21de9680eb59 ("x86emul: replace further UB shifts")
Oss-fuzz: 71138
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index a0deafbd3b99093372f74c09cfb50141416368fa..902538267051511c74a6e7acee5fef0edc9c113f 100644 (file)
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -6369,11 +6369,11 @@ x86_emulate(
         {
             if ( (vex.w ? mask.qw[i] : mask.dw[i]) < 0 )
             {
-                signed long idx = b & 1 ? index.qw[i] : index.dw[i];
+                unsigned long idx = b & 1 ? index.qw[i] : index.dw[i];
 
                 rc = ops->read(ea.mem.seg,
                                truncate_ea(ea.mem.off +
-                                           idx * (1 << state->sib_scale)),
+                                           (idx << state->sib_scale)),
                                (void *)mmvalp + i * op_bytes, op_bytes, ctxt);
                 if ( rc != X86EMUL_OKAY )
                 {
@@ -6489,14 +6489,14 @@ x86_emulate(
 
         for ( i = 0; op_mask; ++i )
         {
-            long idx = b & 1 ? index.qw[i] : index.dw[i];
+            unsigned long idx = b & 1 ? index.qw[i] : index.dw[i];
 
             if ( !(op_mask & (1 << i)) )
                 continue;
 
             rc = ops->read(ea.mem.seg,
                            truncate_ea(ea.mem.off +
-                                       idx * (1 << state->sib_scale)),
+                                       (idx << state->sib_scale)),
                            (void *)mmvalp + i * op_bytes, op_bytes, ctxt);
             if ( rc != X86EMUL_OKAY )
             {
@@ -6643,9 +6643,9 @@ x86_emulate(
 
         for ( i = 0; op_mask; ++i )
         {
-            long idx = (b & 1 ? index.qw[i]
-                              : index.dw[i]) * (1 << state->sib_scale);
-            unsigned long offs = truncate_ea(ea.mem.off + idx);
+            unsigned long idx = b & 1 ? index.qw[i] : index.dw[i];
+            unsigned long offs = truncate_ea(ea.mem.off +
+                                             (idx << state->sib_scale));
             unsigned int j, slot;
 
             if ( !(op_mask & (1 << i)) )
@@ -6663,11 +6663,10 @@ x86_emulate(
              */
             for ( j = (slot = i) + 1; j < n; ++j )
             {
-                long idx2 = (b & 1 ? index.qw[j]
-                                   : index.dw[j]) * (1 << state->sib_scale);
-
+                idx = b & 1 ? index.qw[j] : index.dw[j];
                 if ( (op_mask & (1 << j)) &&
-                     truncate_ea(ea.mem.off + idx2) == offs )
+                     truncate_ea(ea.mem.off +
+                                 (idx << state->sib_scale)) == offs )
                     slot = j;
             }