CTSRD-CHERI / llvm-project

Fork of LLVM adding CHERI support
46 stars 39 forks source link

[CHERIoT] CHERIOT R_RISCV_CHERI_COMPARTMENT_SIZE relocation out of range error #696

Open ajrn1998 opened 1 year ago

ajrn1998 commented 1 year ago

CHERIOT linker fails with relocation out of range error when building the nettle-aes embench benchmark: https://github.com/embench/embench-iot/blob/d9b30cdf805133bef9db5f7ecf84ae1ce8124291/src/nettle-aes/nettle-aes.c#L52-L59

The problem can be more simply reproduced by just declaring a big structure type like aes_table and initilise a struct of this type such as _aes_encrypt_table: https://github.com/embench/embench-iot/blob/d9b30cdf805133bef9db5f7ecf84ae1ce8124291/src/nettle-aes/nettle-aes.c#L88-L389. By simply switching the AES_TABLE_SIZE #define to 2, the linker succeeds.

It seems that the relocation is always set to use the CSetBoundsImm instruction with a max 12 bits immediate. Is there a reason why it would not use the CSetBounds instruction and relax only when it is possible to fit it in the CSetBoundsImm instruction?

linker_error

davidchisnall commented 1 year ago

This is a known (but not documented, sorry!) limitation. Unfortunately, this is a slightly different shape of relaxation to anything currently done and so it's not something that we want to add until we've rebased on a newer version of LLVM that has the upstream support for linker relaxations merged so that we do not diverge too far. The relaxation sequence is complex because there are a bunch of cases to consider:

We would have to emit the third form (which then requires two new relocation types) and relax to one of the others and this kind of relocation pattern is not yet well supported in lld.

In general, compartmentalised code that respects the principle of least privilege rarely has single globals that are over 4 KiB (we've only seen it in one other place so far) and I'd consider it a sign of potential bugs. It looks as if the AES code that you're using places all of the s-boxes in an array. The mBedTLS version (which we're using) places them in separate globals, which gives finer-grained bounds checking and is probably a better approach. In particular, unless you enable sub-object bounds, you won't get bounds errors if the AES implementation walks off one s-box and into the next one with the code structured this way.

Even implementing this relaxation is not ideal because it adds an extra live register, which causes worse code generation elsewhere (and often stack spills and restores), so we'd probably want to guard this behind a flag to avoid making code worse in the common case.