[Bug][Clang][ARM] Wrong assembly code generation, "LR" corrupted. #80287

P1119r1m · 2024-02-01T12:50:49Z

The Clang compiler generates code for a function that overwrites the lr register without saving its data on stack beforehand.
This bug was detected on a large software component and the problem was minimized to a PoC (see example.c).

Clang versions info:

llvmorg-17-init - bug wasn't detected
llvmorg-17.0.2 - bug detected
llvmorg-17.0.6 - bug detected
llvmorg-18-init - bug detected

Host OS: Ubuntu 20.04.3 LTS

In disassembler the bug looks like this (generated example.c.o.objdump):

00000010 <BUGGY_FUNCTION>:
;     if (!ctx) {
      10:     	cmp	r0, #0
      14:     	beq	0x48 <BUGGY_FUNCTION+0x38> @ imm = #0x2c                <-- Goto 0x48 if "(!ctx)" is True.
;     ctx->cmd_c = func_2;
      18:     	ldr	r12, [pc, #0x40]        @ 0x60 <BUGGY_FUNCTION+0x50>    <-- Continue if "(!ctx)" is False.
      1c:     	mov	r1, r0
      20:     	mov	r0, #0
;     ctx->cmd_a = func_0;
      24:     	add	lr, r1, #8              <-- ERROR. THIS LOOKS CRAZY!!! "lr" WAS NOT STORED ON THE STACK!
;     ctx->cmd_c = func_2;
      28:     	ldr	r12, [pc, r12]
... < SOME OTHER CODE > ...
      44:     	bx	lr                      <-- ERROR. BRANCH TO "lr" ADDRESS THAT WAS PREVIOUSLY CORRUPTED!!!
      48:     	push	{r11, lr}
... < SOME OTHER CODE > ...

Steps to reproduce.

example.c

void __attribute__((optnone)) printer(int line) {}

#define INFO() printer(__LINE__);

extern int func_0(void);
extern int func_1(void);
extern int func_2(void);

typedef int (*f_ptr_t)(void);

typedef struct operation {
    double double_0;
    f_ptr_t cmd_a;
    f_ptr_t cmd_b;
    f_ptr_t cmd_c;
    f_ptr_t cmd_d;
} operation_t;

int BUGGY_FUNCTION(operation_t *ctx)
{
    // INFO(); // UNCOMMENT TO FIX ASSEMBLY CODE GENERATION!!!
    if (!ctx) {
        INFO()
        return -1;
    }
    ctx->cmd_a = func_0;
    ctx->cmd_b = func_1;
    ctx->cmd_c = func_2;
    ctx->cmd_d = (void *)0;
    return 0;
}

reproduce.sh

#!/bin/bash

SDK_BIN_PATH=<PUT YOUR BIN PATH LOCATION HERE>

CC="${SDK_BIN_PATH}/armv7-none-gnueabi-clang"
OBJDUMP="${SDK_BIN_PATH}/armv7-none-gnueabi-objdump"

INPUT_FILE=example.c
OBJ_FILE=example.c.o
OBJDUMP_FILE=example.c.o.objdump

echo "Compile and objdump..." \
&& "${CC}" -ggdb -Os -fPIE -c "${INPUT_FILE}" -o "${OBJ_FILE}" \
&& "${OBJDUMP}" -dS "${OBJ_FILE}" | tee "${OBJDUMP_FILE}"

The text was updated successfully, but these errors were encountered:

jroelofs · 2024-02-01T14:31:49Z

Smells like shrink-wrapping.

llvmbot · 2024-02-01T14:31:52Z

@llvm/issue-subscribers-backend-arm

Author: Victor Signaevskyi (P1119r1m)

The Clang compiler generates code for a function that overwrites the `lr` register without saving its data on stack beforehand. This bug was detected on a large software component and the problem was minimized to a PoC (see `example.c`).

Clang versions info:

llvmorg-17-init - bug wasn't detected
llvmorg-17.0.2 - bug detected
llvmorg-17.0.6 - bug detected
llvmorg-18-init - bug detected

Host OS: Ubuntu 20.04.3 LTS

In disassembler the bug looks like this (generated example.c.o.objdump):

00000010 &lt;BUGGY_FUNCTION&gt;:
;     if (!ctx) {
      10:     	cmp	r0, #<!-- -->0
      14:     	beq	0x48 &lt;BUGGY_FUNCTION+0x38&gt; @ imm = #<!-- -->0x2c                &lt;-- Goto 0x48 if "(!ctx)" is True.
;     ctx-&gt;cmd_c = func_2;
      18:     	ldr	r12, [pc, #<!-- -->0x40]        @ 0x60 &lt;BUGGY_FUNCTION+0x50&gt;    &lt;-- Continue if "(!ctx)" is False.
      1c:     	mov	r1, r0
      20:     	mov	r0, #<!-- -->0
;     ctx-&gt;cmd_a = func_0;
      24:     	add	lr, r1, #<!-- -->8              &lt;-- ERROR. THIS LOOKS CRAZY!!! "lr" WAS NOT STORED ON THE STACK!
;     ctx-&gt;cmd_c = func_2;
      28:     	ldr	r12, [pc, r12]
... &lt; SOME OTHER CODE &gt; ...
      44:     	bx	lr                      &lt;-- ERROR. BRANCH TO "lr" ADDRESS THAT WAS PREVIOUSLY CORRUPTED!!!
      48:     	push	{r11, lr}
... &lt; SOME OTHER CODE &gt; ...

Steps to reproduce.

example.c

void __attribute__((optnone)) printer(int line) {}

#define INFO() printer(__LINE__);

extern int func_0(void);
extern int func_1(void);
extern int func_2(void);

typedef int (*f_ptr_t)(void);

typedef struct operation {
    double double_0;
    f_ptr_t cmd_a;
    f_ptr_t cmd_b;
    f_ptr_t cmd_c;
    f_ptr_t cmd_d;
} operation_t;

int BUGGY_FUNCTION(operation_t *ctx)
{
    // INFO(); // UNCOMMENT TO FIX ASSEMBLY CODE GENERATION!!!
    if (!ctx) {
        INFO()
        return -1;
    }
    ctx-&gt;cmd_a = func_0;
    ctx-&gt;cmd_b = func_1;
    ctx-&gt;cmd_c = func_2;
    ctx-&gt;cmd_d = (void *)0;
    return 0;
}

reproduce.sh

#!/bin/bash

SDK_BIN_PATH=&lt;PUT YOUR BIN PATH LOCATION HERE&gt;

CC="${SDK_BIN_PATH}/arm-secureos-gnueabi-clang"
OBJDUMP="${SDK_BIN_PATH}/arm-secureos-gnueabi-objdump"

INPUT_FILE=example.c
OBJ_FILE=example.c.o
OBJDUMP_FILE=example.c.o.objdump

echo "Compile and objdump..." \
&amp;&amp; "${CC}" -ggdb -Os -fPIE -c "${INPUT_FILE}" -o "${OBJ_FILE}" \
&amp;&amp; "${OBJDUMP}" -dS "${OBJ_FILE}" | tee "${OBJDUMP_FILE}"

fhahn · 2024-02-01T14:51:05Z

I suspect this was fixed by swiftlang@b1a5ee1

Could you check with a build from current main or the upcoming 18.x release candidates?

davemgreen · 2024-02-01T14:58:27Z

I still see the same in https://godbolt.org/z/7PKxqj7vx I'm afraid.

It looks like what @jroelofs says - shrink wrapping leads to frame lowering in one of the two branches, followed by a bit of tail folding ends up with the load/store optimizing using lr that it thinks is available.

P1119r1m · 2024-02-01T15:47:51Z

BTW, there is no bug if to build it for -target armv4t-none-eabi instead of -target thumbv7a-none-eabi:

https://godbolt.org/z/5qx39eWxs

P1119r1m · 2024-02-05T07:45:23Z

Dear contributors/collaborators,

are there any chance to fix this compiler bug?

There is a feeling that this bug can manifest itself in many other places in the code.
Maybe not even only in 32-bit architectures.

Also, I think this bug could be used in some hacker attacks.

P1119r1m · 2024-02-21T09:54:16Z

@jroelofs @davemgreen @fhahn
I think we should report a CVE for this Clang issue to warn at least all ARM32 users about this Clang compiler vulnerability.

smithp35 · 2024-02-21T10:11:22Z

If you feel like this is a security issue please report using the process in https://llvm.org/docs/Security.html . My instinct is that this would not be a worthy of a CVE. CVEs are used to report security vulnerabilities in programs, this looks like a code-generation bug that if it affects your program it is likely to crash. It doesn't look like something that would be systemicly reproduced across many programs.

P1119r1m · 2024-02-21T10:15:37Z

@smithp35 Thanks for the link.
I think the bug deserves a CVE because "security experts" can intentionally look for this incorrect binary pattern in binaries and exploit it.
Theoretically, thanks to this compiler bug, one can branch to any address.

smithp35 · 2024-02-21T10:17:27Z

Feel free to report it to the LLVM security group if you disagree with me.

P1119r1m · 2024-02-21T11:08:07Z

@smithp35 Thank you. Reported locally to LLVM security group as:

Issue 69: Wrong assembly code generation. Branching to the corrupted "LR".

jroelofs · 2024-02-21T16:08:24Z

workaround: -mllvm --enable-shrink-wrap=false

https://godbolt.org/z/3Koq6YEWW

This test shows the bug where LR is used as a general-purpose register on a code path where it is not spilled to the stack.

PR llvm#75527 fixed ARMFrameLowering to set the IsRestored flag for LR based on all of the return instructions in the function, not just one. However, there is also code in ARMLoadStoreOptimizer which changes return instructions, but it set IsRestored based on the one instruction it changed, not the whole function. The fix is to factor out the code added in llvm#75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes llvm#80287.

This test shows the bug where LR is used as a general-purpose register on a code path where it is not spilled to the stack.

PR llvm#75527 fixed ARMFrameLowering to set the IsRestored flag for LR based on all of the return instructions in the function, not just one. However, there is also code in ARMLoadStoreOptimizer which changes return instructions, but it set IsRestored based on the one instruction it changed, not the whole function. The fix is to factor out the code added in llvm#75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes llvm#80287.

PR #75527 fixed ARMFrameLowering to set the IsRestored flag for LR based on all of the return instructions in the function, not just one. However, there is also code in ARMLoadStoreOptimizer which changes return instructions, but it set IsRestored based on the one instruction it changed, not the whole function. The fix is to factor out the code added in #75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes #80287.

ostannard · 2024-02-27T13:03:23Z

/cherry-pick 8779cf6 749384c

This test shows the bug where LR is used as a general-purpose register on a code path where it is not spilled to the stack. (cherry picked from commit 8779cf6)

PR llvm#75527 fixed ARMFrameLowering to set the IsRestored flag for LR based on all of the return instructions in the function, not just one. However, there is also code in ARMLoadStoreOptimizer which changes return instructions, but it set IsRestored based on the one instruction it changed, not the whole function. The fix is to factor out the code added in llvm#75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes llvm#80287. (cherry picked from commit 749384c)

llvmbot · 2024-02-27T13:08:20Z

/pull-request #83129

This test shows the bug where LR is used as a general-purpose register on a code path where it is not spilled to the stack. (cherry picked from commit 8779cf6)

PR llvm#75527 fixed ARMFrameLowering to set the IsRestored flag for LR based on all of the return instructions in the function, not just one. However, there is also code in ARMLoadStoreOptimizer which changes return instructions, but it set IsRestored based on the one instruction it changed, not the whole function. The fix is to factor out the code added in llvm#75527, and also call it from ARMLoadStoreOptimizer if it made a change to return instructions. Fixes llvm#80287. (cherry picked from commit 749384c)

tstellar · 2024-04-11T21:06:15Z

Do we have a conclusion about which versions of LLVM and which targets this affects? Does it affect 64-bit ARM?

tstellar · 2024-04-11T21:24:51Z

Do we have a conclusion about which versions of LLVM and which targets this affects? Does it affect 64-bit ARM?

The fix is only for 32-bit ARM, so I'm assuming it doesn't affect 64-bit ARM.

github-actions bot added the clang Clang issues not falling into any other category label Feb 1, 2024

jroelofs added the backend:ARM label Feb 1, 2024

EugeneZelenko removed the clang Clang issues not falling into any other category label Feb 1, 2024

ostannard self-assigned this Feb 22, 2024

ostannard added a commit to ostannard/llvm-project that referenced this issue Feb 23, 2024

Pre-commit test showing bug llvm#80287

3a73ee4

This test shows the bug where LR is used as a general-purpose register on a code path where it is not spilled to the stack.

ostannard mentioned this issue Feb 23, 2024

[ARM] Update IsRestored for LR based on all returns #82745

Merged

ostannard added a commit that referenced this issue Feb 26, 2024

Pre-commit test showing bug #80287

8779cf6

This test shows the bug where LR is used as a general-purpose register on a code path where it is not spilled to the stack.

ostannard closed this as completed in #82745 Feb 26, 2024

ostannard added this to the LLVM 18.X Release milestone Feb 27, 2024

github-project-automation bot added this to LLVM Release Status Feb 27, 2024

github-project-automation bot moved this to Needs Triage in LLVM Release Status Feb 27, 2024

EugeneZelenko added the release:backport label Feb 27, 2024

tstellar moved this from Needs Triage to Done in LLVM Release Status Feb 27, 2024

pointhex mentioned this issue May 7, 2024

getStyleDiagHandler #91314

Closed

aemerson mentioned this issue May 9, 2024

release/18.x: [AArc64][GlobalISel] Fix legalizer assert for G_INSERT_VECTOR_ELT - manual merge #91672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug][Clang][ARM] Wrong assembly code generation, "LR" corrupted. #80287

[Bug][Clang][ARM] Wrong assembly code generation, "LR" corrupted. #80287

P1119r1m commented Feb 1, 2024 •

edited

Loading

jroelofs commented Feb 1, 2024

llvmbot commented Feb 1, 2024

fhahn commented Feb 1, 2024

davemgreen commented Feb 1, 2024

P1119r1m commented Feb 1, 2024

P1119r1m commented Feb 5, 2024

P1119r1m commented Feb 21, 2024

smithp35 commented Feb 21, 2024

P1119r1m commented Feb 21, 2024

smithp35 commented Feb 21, 2024

P1119r1m commented Feb 21, 2024

jroelofs commented Feb 21, 2024

ostannard commented Feb 27, 2024

llvmbot commented Feb 27, 2024

tstellar commented Apr 11, 2024

tstellar commented Apr 11, 2024

[Bug][Clang][ARM] Wrong assembly code generation, "LR" corrupted. #80287

[Bug][Clang][ARM] Wrong assembly code generation, "LR" corrupted. #80287

Comments

P1119r1m commented Feb 1, 2024 • edited Loading

jroelofs commented Feb 1, 2024

llvmbot commented Feb 1, 2024

fhahn commented Feb 1, 2024

davemgreen commented Feb 1, 2024

P1119r1m commented Feb 1, 2024

P1119r1m commented Feb 5, 2024

P1119r1m commented Feb 21, 2024

smithp35 commented Feb 21, 2024

P1119r1m commented Feb 21, 2024

smithp35 commented Feb 21, 2024

P1119r1m commented Feb 21, 2024

jroelofs commented Feb 21, 2024

ostannard commented Feb 27, 2024

llvmbot commented Feb 27, 2024

tstellar commented Apr 11, 2024

tstellar commented Apr 11, 2024

P1119r1m commented Feb 1, 2024 •

edited

Loading