Re: UBSan bug in real mode fpu emulation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 24/04/2025 2:13 pm, Fabian Specht wrote:
>>> if ( !s->rex_prefix )
>>> {
>>>     /* Convert 32-bit real/vm86 to 32-bit prot format. */
>>>     unsigned int fip = fpstate.env.mode.real.fip_lo +
>>>                                        (fpstate.env.mode.real.fip_hi << 16);
>>>     unsigned int fdp = fpstate.env.mode.real.fdp_lo +
>>>                                        (fpstate.env.mode.real.fdp_hi << 16);
>>>     unsigned int fop = fpstate.env.mode.real.fop;
>>>
>>>     fpstate.env.mode.prot.fip = fip & 0xf;
>>>     fpstate.env.mode.prot.fcs = fip >> 4;
>>>     fpstate.env.mode.prot.fop = fop;
>>>     fpstate.env.mode.prot.fdp = fdp & 0xf;
>>>     fpstate.env.mode.prot.fds = fdp >> 4;
>>> }
>> Several things.  First, please always the UBSAN analysis from the crash.
> (XEN) UBSAN: Undefined behaviour in arch/x86/x86_emulate/blk.c:87:66
> (XEN) left shift of 65535 by 16 places cannot be represented in type 'int'
> (XEN) ----[ Xen-4.20.0  x86_64  debug=y ubsan=y  Tainted:     H  ]----

Yes, it is a shift into the sign bit.

>
>> There are several different ways that shifts go wrong, and I suspect
>> this is a shift into a sign bit, which is notable given the unsigned
>> underlying type.
> Might be, but I am not entirely sure. Either way, it should be fixed
> through a simple cast to unsigned int I think.

I have a sneaking suspicion that this is sufficient:

diff --git a/xen/arch/x86/x86_emulate/private.h
b/xen/arch/x86/x86_emulate/private.h
index 30be59547032..9f3d6f0e5357 100644
--- a/xen/arch/x86/x86_emulate/private.h
+++ b/xen/arch/x86/x86_emulate/private.h
@@ -385,9 +385,9 @@ struct x87_env16 {
     union {
         struct {
             uint16_t fip_lo;
-            uint16_t fop:11, :1, fip_hi:4;
+            uint32_t fop:11, :1, fip_hi:4;
             uint16_t fdp_lo;
-            uint16_t :12, fdp_hi:4;
+            uint32_t :12, fdp_hi:4;
         } real;
         struct {
             uint16_t fip;


The problem is that a uint16_t bitfield promotes into int.  A base type
of uint32_t should cause the bitfield to promote into unsigned int directly.

>
>> Also, are you aware that the test isn't properly in Real Mode?  It's in
>> so-called unreal mode (not actually a real mode, but a consequence of
>> how the segment registers work), which is relevant to how you manage to
>> re-enter the emulator for FLDENV.
> Yes I am aware. But the bug should be triggered regardless of the
> current mode, right?

Well, that depends.  Some CPUs trucate %eip to %ip at this point.  Xen
doesn't, which is why the XTF test doesn't crash in a different way.

~Andrew