Re: UBSan bug in real mode fpu emulation

[Manuel Andreas <manuel.andreas@xxxxxx>]

On 4/25/25 11:11, Jan Beulich wrote:
> On 24.04.2025 16:04, Andrew Cooper wrote:
> > 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.
> I fear that's not gcc's way of thinking. In fact, the other involved structure
> already uses bitfields with uint32_t base type, and the issue was reported
> there nevertheless. Having known only compilers which respect declared type of
> bitfields, I was rather surprised by gcc not doing so when I first started
> using it on not exactly trivial code. Just to learn that they are free to do
> so. Looks like I never dared to submit a patch I've been carrying to optionally
> alter that behavior.
So I took some time to play around with this and you're definitely right 
about GCC not respecting the declared type. Even in the struct 
`x87_env32`, where the types are already declared as `uint32_t`, GCC 
will just pick `short unsigned int` as the type for a 16-bit wide 
bit-field. As such, in the offending left-shift expression the bit-field 
will be promoted to an `int`, thereby causing the observed UB.
I could not find a way to make GCC actually pick a correctly sized 
unsigned type in this context, without also modifying the width of the 
bit-field. So I'm relatively sure the only way to fix this is to 
properly adjust the usages of these bit-fields (as was suggested 
previously).
For completeness sake I also checked how Clang deals with this issue and 
the UB manifests in the same manner.
What's worse is that I didn't find any proper documentation on this 
behavior for neither GCC or Clang. If you have anything I would be very 
interested to know.
Best,
Manuel