CHERIoT-Platform / llvm-project

Fork of LLVM adding CHERIoT, based on the CHERI LLVM fork
4 stars 6 forks source link

Warn if compartment call returns void #34

Open rmn30 opened 4 months ago

rmn30 commented 4 months ago

It would be good if clang would warn if the return type of a compartment call is void. Compartment calls can fail unexpectedly and return -1 (e.g. due to trusted stack exhaustion). Callers should be aware of and check for this, therefore it is never appropriate for a compartment call to return void. For the same reason we should also consider automatically adding the nodiscard attribute to compartment calls.

v01dXYZ commented 2 weeks ago

Hi, I'm working on this. How to deal with C++ constructors with a cheri_compartment attr ?

v01dXYZ commented 1 week ago

I think it is worth to take the c++ cases into account (non static method, constructor/destructor, lambda ?, etc.). But it would be better to write another ticket for that.

davidchisnall commented 1 week ago

I'm not sure what it would even mean to expose a constructor across a compartment boundary. A factory method, maybe, but what would the security model be for a constructor? Non-static methods can't be compartment exports, because they need to in the vtable and the ABI does not allow that. It would be possible to make a lambda's call operator a cross-compartment call, but there's no way of exposing that in the source language.

v01dXYZ commented 1 week ago

I'll open a ticket about generating an error when non static methods are used as compartment entry points. (Or you can if you have other edge cases in mind).

There are more things to mention as I progress:

davidchisnall commented 1 week ago

To the first, yes this makes no sense for function pointers. Function pointers must use the Cheri callback convention. They are not (and cannot be) explicitly tied to a compartment.

I think showing the warning twice to start with is fine for a first cut.

v01dXYZ commented 1 week ago

I wrote a first version for cheri_compartment. I reported the duplicate attribute problem caused by GetTypeForDeclarator which is the root cause of the previous problem (https://github.com/llvm/llvm-project/issues/116543).

There are still to support cheri_ccall and cheri_ccallback. But this is a little bit more complicated than expected as the attribute is added to the AST in a different part where it's more difficult to also add an additional WarnUnusedResult attr. Emitting a diagnosis about void return type could easily be done though.