Skip to content

Commit

Permalink
Avoid matching prefixes of a symbol as arm registers (#1807)
Browse files Browse the repository at this point in the history
### Issues:

Related to CryptoAlg-2625

### Description of changes: 

In some cases the peg parser incorrectly classify a prefix of a symbol
reference instruction argument as an arm register. This causes the
delocator to miss required re-writes.

The offending rule is:
```
ARMRegister <- [..] / ([xwdqshb] [0-9] [0-9]?) / [...]
```  
e.g. the register `x2` will match this rules, as it should. However, due
to how the rest of the rules are formed this can cause strings such as
`x25519_..` to match the first prefix, when the string is an instruction
argument, causing a wrong classification of tokens on a line. The parser
is not architecture-aware, so this cause errors on other archs as well.

This was fine until now, because we had no symbols that required
re-writes with a matching prefix. But to move curve25519 functions into
the fips module, this assumption is no longer true. For example,
`x25519_ge_frombytes_vartime` is one such function. Note we can't reduce
the visibility of this symbol, because it's referenced in a compilation
unit outside the fips module bcm.o (in `spake25519.c`).

The required re-write is due to relocations emitted for global symbols.
The re-writing works as follow. Given a global symbol and a reference
(inside fips module):
```
  .globl x25519_foo
x25519_foo:

[...]

bar:
  call x25519_foo
```

The correct delocater behaviour would be a re-write to `*_local_target`
(suffix hardcoded in delocater):
```
  .globl x25519_foo
.Lx25519_foo_local_target
x25519_foo:

[...]

bar:
  call .Lx25519_foo_local_target
```

But in the current behaviour, the parser classify the prefix `x25` of
the instruction argument `x25519_foo` as an arm register. Subsequently,
the delocater ignores ` call x25519_foo`. The local target
`.Lx25519_foo_local_target` is emitted, but the instruction argument is
not re-written. That is, the following happens:
```
  .globl x25519_foo
.Lx25519_foo_local_target
x25519_foo:

[...]

bar:
  call x25519_foo
```
and a relocation is emitted, corrupting the FIPS integrity check at
run-time.

### Fix:

For the offending matching rule, we add an additional look-ahead
condition `!(ARMRegisterBoundary)`. Any matching on `[xwdqshb] [0-9]
[0-9]?` must not contain a suffix of `ARMRegisterBoundary`.

### Testing:

Added a test that global symbols with matching prefixing, e.g.
`x25519_foo`, are re-written correctly.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.
  • Loading branch information
torben-hansen authored Aug 27, 2024
1 parent 519c1c5 commit 818091f
Show file tree
Hide file tree
Showing 4 changed files with 1,139 additions and 1,064 deletions.
3 changes: 2 additions & 1 deletion util/fipstools/delocate/delocate.peg
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,11 @@ RegisterOrConstant <- (('%'[[A-Z]][[A-Z0-9]]*) /
![fb:(+\-]
ARMConstantTweak <- ((([us] "xt" [xwhb]) / "lsl" / "lsr" / "ror" / "asr") (WS '#'? Offset)?)/
"mul vl" # multiply offset by the hardware's vector length
ARMRegister <- "sp" / ([xwdqshb] [0-9] [0-9]?) / "xzr" / "wzr" / "NZCV" / ARMVectorRegister / SVE2PredicateRegister /
ARMRegister <- "sp" / ([xwdqshb] [0-9] [0-9]? !(ARMRegisterBoundary)) / "xzr" / "wzr" / "NZCV" / ARMVectorRegister / SVE2PredicateRegister /
('{' WS? ARMVectorRegister WS? ([,\-] WS? ARMVectorRegister)* WS? '}' ('[' [0-9] [0-9]? ']')? )
ARMVectorRegister <- [vz] [0-9] [0-9]? ('.' [0-9]* [bsdhq] ('[' [0-9] [0-9]? ']')? )?
SVE2PredicateRegister <- "p" [0-9] [0-9]? "/" [mMzZ]
ARMRegisterBoundary <- [a-zA-Z0-9_]
# Compilers only output a very limited number of expression forms. Rather than
# implement a full expression parser, this enumerate those forms plus a few
# that appear in our hand-written assembly.
Expand Down
Loading

0 comments on commit 818091f

Please sign in to comment.