Buffer overflow at X86FloatingPoint.cpp

[DyadyaMops]

for (unsigned I = 0; I < N; ++I)
    pushReg(N - I - 1);

There is a buffer overflow error in the code, since the Reg value may exceed the size of the RegMap array, which leads to an outflow of the array boundaries. Checking for stackTop (see below) does not protect against this case, since it checks only the Stack array, not the RegMap. It is necessary to add a check of the range of Reg values before calling pushReg to make sure that Reg is within acceptable limits.

 void pushReg(unsigned Reg) {
      assert(Reg < NumFPRegs && "Register number out of range!");
      if (StackTop >= 8)
        report_fatal_error("Stack overflow!");
      Stack[StackTop] = Reg;
      RegMap[Reg] = StackTop++;
    }

[llvmbot]

@llvm/issue-subscribers-backend-x86

Author: None (DyadyaMops)

```cpp for (unsigned I = 0; I < N; ++I) pushReg(N - I - 1); ``` There is a buffer overflow error in the code, since the Reg value may exceed the size of the RegMap array, which leads to an outflow of the array boundaries. Checking for stackTop (see below) does not protect against this case, since it checks only the Stack array, not the RegMap. It is necessary to add a check of the range of Reg values before calling pushReg to make sure that Reg is within acceptable limits. ```cpp void pushReg(unsigned Reg) { assert(Reg < NumFPRegs && "Register number out of range!"); if (StackTop >= 8) report_fatal_error("Stack overflow!"); Stack[StackTop] = Reg; RegMap[Reg] = StackTop++; } ```

[RKSimon]

Do you have a repro test case for this? I can see it can technically occur in FPS::handleCall if STReturns == 0, but don't have a test case to hand.

[topperc]

Wouldn't the assert(Reg < NumFPRegs && "Register number out of range!"); fail?

[RKSimon]

I think the query is why we just assert for that failed condition, but use report_fatal_error for the other

[topperc]

Do you have a repro test case for this? I can see it can technically occur in FPS::handleCall if STReturns == 0, but don't have a test case to hand.

If STReturns is 0, then N is 0, so the pushReg loop doesn't run right?

[RKSimon]

Sorry misread this as countr_zero

https://github.com/llvm/llvm-project/blob/89c26f6c7b0a6dfa257ec090fcf5b6e6e0c89aab/llvm/lib/Target/X86/X86FloatingPoint.cpp#L1049-L1062

@DyadyaMops Its back to you to provide a test case.

[DyadyaMops]

@RKSimon The fact is that it was found using static analysis, and not in dynamics, so I don't have any test cases.

[RKSimon]

Can you post a more complete summary of the analysis report ?

[DyadyaMops]

Sure. Here is the description of the found issue:

Value 31 is used in function '(anonymous namespace)::FPS::pushReg' at X86FloatingPoint.cpp:1005 to calculate an index for accessing an element of array of size 8. This may lead to a buffer overflow. The out of range index value is originated from X86FloatingPoint.cpp:1005.

Vulnerability trace (as the analyzer reasoned):

Trace 1.1 Call of '(anonymous namespace)::FPS::pushReg'Index must be less than 8

for (unsigned I = 0; I < N; ++I)
    pushReg(N - I - 1);
}

Trace 1.2 Out-of-bounds access to buffer 'this->RegMap'

void pushReg(unsigned Reg) {
      assert(Reg < NumFPRegs && "Register number out of range!");
      if (StackTop >= 8)
        report_fatal_error("Stack overflow!");
      Stack[StackTop] = Reg;
      RegMap[Reg] = StackTop++;
    }

Trace 2.2 (Sub: N - I - 1) >= 8 Trace 2.3 (Sub: N - I) >= 17

pushReg(N - I - 1);

Trace 2.4 Assign '32' to '*'

MathExtras.h

#if __GNUC__ >= 4 || defined(_MSC_VER)
template <typename T> struct TrailingZerosCounter<T, 4> {
  static unsigned count(T Val, ZeroBehavior ZB) {
    if (ZB != ZB_Undefined && Val == 0)
      return 32;

Trace 2.5 Call of 'llvm::countTrailingOnes'

X86FloatingPoint.cpp

 unsigned N = countTrailingOnes(STReturns);

Trace 2.6 Call of 'llvm::countTrailingZeros' >= 24

template <typename T>
unsigned countTrailingOnes(T Value, ZeroBehavior ZB = ZB_Width) {
  static_assert(std::numeric_limits<T>::is_integer &&
                    !std::numeric_limits<T>::is_signed,
                "Only unsigned integral types are allowed.");
  return countTrailingZeros<T>(~Value, ZB);
}