[Bug target/115014] New: GCC generates incorrect instructions for addressing the data segment through EBP register

[Bug target/115014] New: GCC generates incorrect instructions for addressing the data segment through EBP register

[mdoucha at suse dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

            Bug ID: 115014
           Summary: GCC generates incorrect instructions for addressing
                    the data segment through EBP register
           Product: gcc
           Version: 13.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mdoucha at suse dot cz
  Target Milestone: ---

Created attachment 58149
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58149&action=edit
Preprocessed reproducer source file

Target: x86 (32bit)

While writing a new KVM test for the Linux Test Project, I ran into some
mysterious virtual machine crashes due to stack segment fault exception. The
test is essentially a tiny kernel that'll bootstrap a virtual CPU into
protected mode, run a few operations to test something and return results in a
predefined block of memory to the controller process running on the host
machine.

The cause of the crashes turns out to be a combination of custom stack segment
limit and GCC generating unprefixed instructions for accessing data segment
addresses through the EBP register. For example, GCC generates this
instruction:
movzbl 0x1(%ebp),%eax

When it should generate a prefixed one instead:
movzbl %ds:0x1(%ebp),%eax

This would not be a problem on most systems because the stack and data segments
are usually identical but due to the embedded nature of the KVM test, the stack
segment has special settings and the address in EBP happens to be a pointer to
string constant stored outside the stack segment limit.

The fix should be simple:
1) If the address in EBP was calculated from stack pointer value, generate
unprefixed instructions.
2) Otherwise add data segment prefix.
This condition should be applied at least if the source file is compiled for
32bit x86 with the -ffreestanding argument.

The attached source file contains a minimal reimplementation of standard C
library functions for the embedded environment. The vsprintf() reimplementation
is where the issue manifests. The source file was compiled with the following
command line options:
gcc -I../testcases/kernel/kvm/include -DCOMPILE_PAYLOAD -ffreestanding -O2
-Wall -fno-asynchronous-unwind-tables -fno-stack-protector -mno-mmx -mno-sse
-save-temps -m32 -fno-pie -c -o lib_guest.o ../testcases/kernel/kvm/lib_guest.c

Note that this is not the full KVM test which you could run to trigger the
stack segment fault exception. The full test would require:
- CPU bootstrap file in assembly[1] (see below)
- the attached reproducer file
- main test source
- test controller source
- custom linker script
- build script to tie it all together

Instead, you can just see the assembly output and look for the movzbl
instruction mentioned above.

The GCC package used for building the KVM test comes from SUSE, but I've
already talked to the SUSE maintainer (Martin Jambor) and he asked me to open
an upstream bug.


gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib64/gcc/x86_64-suse-linux/13/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-suse-linux
Configured with: ../configure CFLAGS=' -O2 -funwind-tables
-fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -g'
CXXFLAGS=' -O2 -funwind-tables -fasynchronous-unwind-tables
-fstack-clash-protection -Werror=return-type -g' XCFLAGS=' -O2 -funwind-tables
-fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -g'
TCFLAGS=' -O2 -funwind-tables -fasynchronous-unwind-tables
-fstack-clash-protection -Werror=return-type -g' GDCFLAGS=' -O2 -funwind-tables
-fasynchronous-unwind-tables -fstack-clash-protection -g' --prefix=/usr
--infodir=/usr/share/info --mandir=/usr/share/man --libdir=/usr/lib64
--libexecdir=/usr/lib64
--enable-languages=c,c++,objc,fortran,obj-c++,ada,go,d,jit,m2
--enable-offload-targets=nvptx-none,amdgcn-amdhsa, --enable-offload-defaulted
--without-cuda-driver --enable-host-shared --enable-checking=release
--disable-werror --with-gxx-include-dir=/usr/include/c++/13
--with-libstdcxx-zoneinfo=/usr/share/zoneinfo --enable-ssp --disable-libssp
--disable-libvtv --enable-cet=auto --disable-libcc1 --enable-plugin
--with-bugurl=https://bugs.opensuse.org/ --with-pkgversion='SUSE Linux'
--with-slibdir=/lib64 --with-system-zlib --enable-libstdcxx-allocator=new
--disable-libstdcxx-pch --enable-libphobos
--enable-version-specific-runtime-libs --with-gcc-major-version-only
--enable-linker-build-id --enable-linux-futex --enable-gnu-indirect-function
--program-suffix=-13 --without-system-libunwind --enable-multilib
--with-arch-32=x86-64 --with-tune=generic
--with-build-config=bootstrap-lto-lean --enable-link-serialization
--build=x86_64-suse-linux --host=x86_64-suse-linux
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 13.2.1 20240206 [revision 67ac78caf31f7cb3202177e6428a46d829b70f23]
(SUSE Linux) 


The CPU bootstrap file for reference, with stack segment setup highlighted:
[1]
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/kvm/bootstrap_x86.S#L117

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2024-05-09
           Severity|normal                      |enhancement
             Status|UNCONFIRMED                 |WAITING

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I am not so sure this is wanted. GCC assumes a flat memory system for stack and
normal memory accesses .

>1) If the address in EBP was calculated from stack pointer value, generate unprefixed instructions.

This fix is not simple really because addresses don't need to be just in EBP
really and even can be stored off into stack memory and then loaded back.

It seems like there must be a better way of implementing this inside your
kernel as a flat memory system between stack and data segments is not going to
change because it would add a very complex tracking.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[mdoucha at suse dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #2 from Martin Doucha <mdoucha at suse dot cz> ---
(In reply to Andrew Pinski from comment #1)
> I am not so sure this is wanted. GCC assumes a flat memory system for stack
> and normal memory accesses .

This is certainly not wanted in the most common case where the flat memory
assumption is correct. But it should be used for compiling embedded binaries
with -ffreestanding argument.

> >1) If the address in EBP was calculated from stack pointer value, generate unprefixed instructions.
> 
> This fix is not simple really because addresses don't need to be just in EBP
> really and even can be stored off into stack memory and then loaded back.
> 
> It seems like there must be a better way of implementing this inside your
> kernel as a flat memory system between stack and data segments is not going
> to change because it would add a very complex tracking.

The worst case for adding prefixes where they're not needed is slight binary
size and possibly performance penalty. The worst case for not adding them is
stack segment exception. The tracking does not need to be perfect.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Martin Doucha from comment #2)
> 
> This is certainly not wanted in the most common case where the flat memory
> assumption is correct. But it should be used for compiling embedded binaries
> with -ffreestanding argument.

I think most embedded targets use a flat memory system even on x86_64. I really
think you are barking up the wrong tree on getting this implemented in GCC (or
even LLVM) because of the complexity of tracking the memory.

It is not just about adding ds for ebp accesses.

Maybe the easiest fix is to use -fno-omit-frame-pointer to force always the use
of EBP as stack accesses only.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[mdoucha at suse dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #4 from Martin Doucha <mdoucha at suse dot cz> ---
(In reply to Andrew Pinski from comment #3)
> I think most embedded targets use a flat memory system even on x86_64. I
> really think you are barking up the wrong tree on getting this implemented
> in GCC (or even LLVM) because of the complexity of tracking the memory.

x86_64 targets have no choice about using flat memory because segment limits
are ignored by hardware in long mode. This issue is 32bit only.

> It is not just about adding ds for ebp accesses.

As far as I can tell, EBP and ESP are the only registers that default to stack
segment. What else would be needed?

> Maybe the easiest fix is to use -fno-omit-frame-pointer to force always the
> use of EBP as stack accesses only.

That's the first thing I've tried and it didn't help. I had to use -ffixed-ebp
as a workaround but that results in a worse performance penalty than prefixes.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xry111 at gcc dot gnu.org

--- Comment #5 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Martin Doucha from comment #2)
> (In reply to Andrew Pinski from comment #1)
> > I am not so sure this is wanted. GCC assumes a flat memory system for stack
> > and normal memory accesses .
> 
> This is certainly not wanted in the most common case where the flat memory
> assumption is correct. But it should be used for compiling embedded binaries
> with -ffreestanding argument.

No, "Freestanding Environment" has a very specific meaning in the C standard. 
If we must add this it'd be a separate -m{something} argument.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[mdoucha at suse dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #6 from Martin Doucha <mdoucha at suse dot cz> ---
(In reply to Xi Ruoyao from comment #5)
> No, "Freestanding Environment" has a very specific meaning in the C
> standard.  If we must add this it'd be a separate -m{something} argument.

I would say that "execution without any benefit of an operating system" does
have some implication for what you can assume about the stack segment,
therefore properly prefixing EBP dereferences should be controlled by the
parameter.

Just to be clear, I'm not asking for support of arbitrary segment base
addresses, not even for the stack segment alone, because that would require
flawless tracking of segment association for every single pointer. What I'm
asking for is that in a freestanding environment, the stack segment should be
treated as a limited subset of the data segment. All stack addresses must be
accessible through the data segment but some data segment addresses may fall
outside the stack segment limit. This will allow embedded code to set up basic
protection against stack overflow without configuring full page translation.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[amonakov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amonakov at gcc dot gnu.org

--- Comment #7 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
I think your report was misunderstood. Anyway, when EBP is not a frame pointer,
I think the chances are low that it is derived from ESP. Would accurate
placement of segment override prefixes be worth the effort compared to
prefixing all EBP-relative accesses with DS override?

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I didn't misunderstanding it. I just think it is not a valid request.
Especially when it comes to it is only this special kvm kernel. 

Someone in theory could also use an offset from esp and run into a similar
failure.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[mdoucha at suse dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #9 from Martin Doucha <mdoucha at suse dot cz> ---
(In reply to Andrew Pinski from comment #8)
> Someone in theory could also use an offset from esp and run into a similar
> failure.

Well yes, but that would be an actual stack overflow and the whole point is to
allow basic protection against that.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
int f(int *a)
{
  int b;
  size_t t = (size_t)&b;
  size_t t1 = (size_t)a;
  return *(int*)(((size_t)&b)+(t-t1));
}

Is kinda of valid c but might fail with your definition.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #11 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #10)
> int f(int *a)
> {
>   int b;
>   size_t t = (size_t)&b;
>   size_t t1 = (size_t)a;
>   return *(int*)(((size_t)&b)+(t-t1));
> }
> 
> Is kinda of valid c but might fail with your definition.

The result of integer-to-pointer conversion is implementation-defined, so it's
not necessarily valid if the implementation has segmented memory.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[mdoucha at suse dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

--- Comment #12 from Martin Doucha <mdoucha at suse dot cz> ---
(In reply to Alexander Monakov from comment #7)
> I think your report was misunderstood. Anyway, when EBP is not a frame
> pointer, I think the chances are low that it is derived from ESP. Would
> accurate placement of segment override prefixes be worth the effort compared
> to prefixing all EBP-relative accesses with DS override?

Expanding on my previous comment (#9), EBP should be preferred to address local
arrays and local variables allocated on the stack. Access to these variables
should not have the DS prefix in order to enforce the stack limit. But trying
to trace segment association through function calls that take a pointer and
return another is not worth the effort.

(In reply to Andrew Pinski from comment #10)
> int f(int *a)
> {
>   int b;
>   size_t t = (size_t)&b;
>   size_t t1 = (size_t)a;
>   return *(int*)(((size_t)&b)+(t-t1));
> }
> 
> Is kinda of valid c but might fail with your definition.

I assume that you're trying to associate the value of pointer "a" with the
stack segment (but that should be (&b)+(t1-t) instead). As you've writtend it
with the type cast to size_t before adding and subtracting, GCC should just
drop any segment association because you're adding plain numbers, not doing
actual pointer arithmetic. Without the typecast (i.e. (&b)+(t1-t)), the result
of the pointer arithmetic is undefined according to C standard section 6.5.6
paragraph 8.

[Bug target/115014] GCC generates incorrect instructions for addressing the data segment through EBP register

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115014

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |i?86-*-*

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Yeah, the C standard only guarantees pointer-to-intptr_t roundtrip for
unaltered values.

I also think this request is too cumbersome to implement for little benefit.