[WIP] pac ty disc#158567
Conversation
rustbot
added
A-compiletest
This comment has been minimized.
This comment has been minimized.
| cv: Scalar, | ||
| layout: abi::Scalar, | ||
| llty: Self::Type, | ||
| schema: Option<PointerAuthSchema>, |
There was a problem hiding this comment.
This function should have access to the Session already through self.tcx.sess.
| fn get_fn_addr( | ||
| &self, | ||
| instance: Instance<'tcx>, | ||
| pointer_auth_schema: Option<PointerAuthSchema>, |
There was a problem hiding this comment.
2d3b257 to
f5fa52a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
d6ce62a to
755b0b7
Compare
This comment has been minimized.
This comment has been minimized.
755b0b7 to
c3fd17f
Compare
This comment has been minimized.
This comment has been minimized.
c3fd17f to
8b6e351
Compare
This comment has been minimized.
This comment has been minimized.
8b6e351 to
5b8ba19
Compare
This comment has been minimized.
This comment has been minimized.
4301abb to
9545275
Compare
| _new_key: u32, | ||
| _new_discriminator: u64, | ||
| ) -> Self::Value { | ||
| bug!("Resigning of pointers not implemented"); |
There was a problem hiding this comment.
Is there a test for this (ensuring that it's not supported) or is it infeasible to reach this path?
There was a problem hiding this comment.
It is not a user facing function, so the only way it could be triggered is if we wrote incorrect code within the compiler itself, hence the bug!. The relevant call chain for this function starts inside codegen_transmute and is guarded by a function pointer support check.
| // Direct function pointer. | ||
| if let Some(input) = build_fn_ptr_type_discriminator_input(tcx, ty) { | ||
| // Only support about extern "C". | ||
| if matches!(input.abi(), ExternAbi::C { .. } | ExternAbi::System { .. }) { |
There was a problem hiding this comment.
Could you please explain what ExternAbi::System stands for? I would appreciate if you also refer me to the corresponding test
There was a problem hiding this comment.
System means the platform’s default system ABI for foreign functions, whatever the platform uses by default for system libraries. I was told in one of the PRs that for all practical cases they should match. Hence the inclusion of both.
The docs.
|
|
||
| let address_space = cx.tcx.global_alloc(prov.alloc_id()).address_space(cx); | ||
| let schema = if cx.sess().pointer_authentication() { | ||
| let mut schema = if cx.sess().pointer_authentication() { |
There was a problem hiding this comment.
Why do we need mut here?
There was a problem hiding this comment.
Because we then retrieve the discriminator and store it in the schema.
| operand: OperandRef<'tcx, Bx::Value>, | ||
| cast: TyAndLayout<'tcx>, | ||
| ) -> OperandValue<Bx::Value> { | ||
| debug!( |
There was a problem hiding this comment.
Is this intended to be included in the PR or is it a development-stage-only debug printing?
There was a problem hiding this comment.
Intended. This serves the same purpose as LLVM_DEBUG(dbgs()...) in llvm.
| pub mod middle; | ||
| pub mod mir; | ||
| pub mod mono; | ||
| pub mod ptrauth; |
There was a problem hiding this comment.
There are no other changes in the file - so, is this actually needed?
There was a problem hiding this comment.
Yes.
This says something along the lines of: "There is a module called ptrauth, and it should be included in the library’s public interface.If you go toptrauthdirectory you'll see themod.rs` which in turns tells us what is publicly exported from the module.
So the flow of visibility is: lib.rs -> ptrauth -> discriminator / llvm_siphash (where the last two are controlled by https://github.com/jchlanda/rust/blob/9545275cededc9e307c61ef9d640d256ae59eab3/compiler/rustc_middle/src/ptrauth/mod.rs#L5)
There was a problem hiding this comment.
This deleted test should also be removed from the markdown documentation file which includes it in test coverage
There was a problem hiding this comment.
Yeap, need to work on that file a bit more.
| pub c_variadic: bool, | ||
|
|
||
| /// Computed type discriminator for pointer authentication purpose. | ||
| pub type_discriminator: u64, |
There was a problem hiding this comment.
It's probably worth prefixing ptrauth-related things with smth like ptrauth_ in such "generic" places (which are not ptrauth-specific). A comment here contains explanation, but when constructing struct objects we have type_discriminator: XXX which non-ptrauth-aware reader would not easily understand
There was a problem hiding this comment.
OK, renamed to ptrauth_type_discriminator.
| fixed_count: self.fixed_count, | ||
| conv: self.conv.stable(tables, cx), | ||
| c_variadic: self.c_variadic, | ||
| type_discriminator: 0, |
There was a problem hiding this comment.
Could you please explain why we have 0 here and are there any cases when non-zero discriminator would be needed here?
There was a problem hiding this comment.
I think you are on to something. There are 3 structs with the same name (FnAbi): public, callconv and gcc. I wonder if we could get away with only extending callconv.
I'll get back to you on this one.
There was a problem hiding this comment.
Yeap, the only one we need is the callconv, refactored.
| fixed_count: 3, | ||
| conv: CanonAbi::C, | ||
| can_unwind: false, | ||
| type_discriminator: 0, |
There was a problem hiding this comment.
Probably deserves a comment that it's just a placeholder value since gcc backend is not supported for pointer authentication (if I got your reasoning right)
| fn_ptr_discriminators.as_ref().and_then(|m| m.get(Size::from_bytes(offset as u64))); | ||
|
|
||
| if let (Some(schema), Some(discr)) = (schema.as_mut(), discr) { | ||
| schema.constant_discriminator = discr as u16; |
There was a problem hiding this comment.
Does this override discriminator value for init/fini if fn ptr type discr is enabled? If so, it looks diverging from how clang handles this - and my understanding is that it actually makes sense to have hard-coded separate discriminator for init/fini since these are (a) known type (b) we need to know discriminator in dynamic loader.
There was a problem hiding this comment.
Are you saying that discriminator for init/fini must always be 0xd9d4, regardless if fn ptr type discrimination is on or not? If so, than we've got a bug, as it would be indeed overridden. Let me see what clang does and come back to you.
There was a problem hiding this comment.
Right, we do have a bug. Clang does:
- no init/fini, no fn ptr ty disc:
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 90, ptr @foo, ptr null }]- init/fini, no fn ptr ty disc:
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 90, ptr ptrauth (ptr @foo, i32 0, i64 55764, ptr inttoptr (i64 1 to ptr)), ptr null }]- no init/fini, fn ptr ty disc:
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 90, ptr @foo, ptr null }]- init/fini, fn ptr ty disc:
@llvm.global_ctors = appending global [1 x { i32, ptr, ptr }] [{ i32, ptr, ptr } { i32 90, ptr ptrauth (ptr @foo, i32 0, i64 55764, ptr inttoptr (i64 1 to ptr)), ptr null }]I'll have to fix that in rust.
There was a problem hiding this comment.
OK, fixed now, extended the test with O{0|3}_PAUTH-INIT-FINI-FN-TY-DISC.
This comment has been minimized.
This comment has been minimized.
697c5ce to
eef2752
Compare
This comment has been minimized.
This comment has been minimized.
fb93d1f to
bff0a2e
Compare
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| // Canonicalize `Option<fn ptr>` to `fn ptr`. This is so that we can express C's null ptr argument. | ||
| // Please see pauth-fn-ptr-type-discrimination-null-arg.rs test for an example. |
There was a problem hiding this comment.
I was unable to find this test. Was it accidentally lost or maybe renamed?
There was a problem hiding this comment.
My bad, got split to different tests:
- pauth-fn-ptr-type-discrimination-option-callback.rs,
- pauth-fn-ptr-type-discrimination-option-return.rs,
- pauth-fn-ptr-type-discrimination-option.rs
| } | ||
| } | ||
|
|
||
| // Canonicalize `Option<fn ptr>` to `fn ptr`. This is so that we can express C's null ptr argument. |
There was a problem hiding this comment.
Is it deliberate choice that we only care about function pointers in this functions? What about data pointers which could also be nullptr - are they represented w/o Option wrapper and such unwrapping is not needed therefore, or they are handled somehow somewhere else, or maybe I'm just missing smth?
There was a problem hiding this comment.
In all honesty I did not think about Rust nullable data pointer representation. Let me research it a bit and come back to you.
There was a problem hiding this comment.
It was an omission, I made it follow function pointer logic, so we will now recover underlying pointer type from Option<* T>. Expanded the test (pauth-fn-ptr-type-discrimination-option-callback.rs) as well.
Done in: 697a1f4
| let x = (A(f), NotFn(999)); | ||
| // DISC: [[Y:%.*]] = alloca [16 x i8] | ||
| // DISC: store ptr ptrauth (ptr @{{.*}}f, i32 0, i64 18983), ptr [[Y]] | ||
| // NO_DISC: [[Y:%.*]] = alloca [16 x i8] | ||
| // NO_DISC: store ptr ptrauth (ptr @{{.*}}f, i32 0), ptr [[Y]] | ||
| let y: (B, AlsoNotFn) = unsafe { mem::transmute(x) }; | ||
|
|
||
| unsafe { | ||
| ptr::read_volatile(&y); | ||
| // DISC: [[Y_LOAD:%.*]] = load ptr, ptr [[Y]] | ||
| // DISC: call void [[Y_LOAD]](i32 42) {{.*}} [ "ptrauth"(i32 0, i64 2712) ] | ||
| // NO_DISC: [[Y_LOAD:%.*]] = load ptr, ptr [[Y]] | ||
| // NO_DISC: call void [[Y_LOAD]](i32 42) {{.*}} [ "ptrauth"(i32 0, i64 0) ] | ||
| (y.0.0)(42); |
There was a problem hiding this comment.
I might be missing smth, but currently this reads as:
- We store function pointer of type
void()to a struct nested into a tuple. - We transmute that to another tuple so the struct now carries
void(i32)instead ofvoid(). BUT: the stored function pointer is still signed with discriminator 18983 standing forvoid()if I'm not mistaken - During function call, we do authentication with discriminator 2712 standing for
void(i32)if I'm not mistaken
So, this smells as auth failure (was it intended?)
And also: should the transmute-handling logic do the resigning (including nested members) so y already carries the pointer signed with 2712 (so transmute first does auth of value from x with discr 18983 and then resign with 2712)?
There was a problem hiding this comment.
You are right, it will result in auth failure. However, I think in this particular case it is desired. My reading of this is that the compiler sees it as a type pun through a pointer cast on the tuple, not a function pointer cast and so does not try to apply the discriminator domain transition.
This is exactly the same behaviour that clang displays. Consider this C sample:
#include <stdint.h>
typedef void (*fn0)(void);
typedef void (*fn1)(int);
void f(void);
void g(int);
typedef struct {
fn0 f;
} A;
typedef struct {
fn1 f;
} B;
typedef struct {
uint64_t x;
} NotFn;
typedef struct {
uint64_t x;
} AlsoNotFn;
typedef struct {
A a;
NotFn n;
} TupleA;
typedef struct {
B b;
AlsoNotFn n;
} TupleB;
__attribute__((noinline)) void test_6_mixed_layout_cast(void) {
TupleA x;
x.a.f = f;
x.n.x = 999;
TupleB y = *(TupleB *)&x;
volatile TupleB tmp = y;
y.b.f(42);
}It compiles to essentially the same IR:
define dso_local void @test_6_mixed_layout_cast() local_unnamed_addr #0 {
%1 = alloca ptr, align 8
%2 = alloca i64, align 8
%3 = alloca ptr, align 8
%4 = alloca i64, align 8
call void @llvm.lifetime.start.p0(ptr nonnull %1)
call void @llvm.lifetime.start.p0(ptr nonnull %2)
store ptr ptrauth (ptr @f, i32 0, i64 18983), ptr %1, align 8, !tbaa !13
store i64 999, ptr %2, align 8, !tbaa !15
call void @llvm.lifetime.start.p0(ptr nonnull %3)
call void @llvm.lifetime.start.p0(ptr nonnull %4)
%5 = load volatile ptr, ptr %1, align 8, !tbaa !13
store volatile ptr %5, ptr %3, align 8, !tbaa !13
%6 = load volatile i64, ptr %2, align 8, !tbaa !15
store volatile i64 %6, ptr %4, align 8, !tbaa !15
tail call void %5(i32 noundef 42) #3 [ "ptrauth"(i32 0, i64 2712) ]
call void @llvm.lifetime.end.p0(ptr nonnull %3)
call void @llvm.lifetime.end.p0(ptr nonnull %4)
call void @llvm.lifetime.end.p0(ptr nonnull %1)
call void @llvm.lifetime.end.p0(ptr nonnull %2)
ret void
}| // Lowering (Rust Ty -> ClangDiscTy) | ||
| fn is_representing_c_complex(fields: &[Ty<'_>]) -> bool { | ||
| fields.len() == 2 && fields[0] == fields[1] && is_complex_compatible_float(fields[0]) | ||
| } |
There was a problem hiding this comment.
I'm wondering if we actually need that. My understanding is that if rust does not have native _Complex type, it's not very likely that it appears in real-world interop with C. And if one needs that - they might adopt different strategies, including this solution involving tuples, but not limited to it.
So my concern is that we are "inventing" smth for a use case which (a) is rare (b) might be handled differently by different parties.
Sure, our "type mangling" logic would not be full-mirror of Clang's if we drop _Complex support, and if we do, it should be properly documented. I just want to raise attention to this decision point - whether we really want this new assumption about tuple of two floats being introduced or maybe just do not support _Complex explicitly and let downstream users which need that (if any) implement this the way they want given that representations might vary.
There was a problem hiding this comment.
That is a valid point. My reasoning was to follow explicitly clang's behaviour with maximum alignment to what can and can not be expressed. I don't have a preference, nor the knowledge of how end users would end up interacting with the types. Perhaps this one is for an offline (off the PR, in person) discussion, it will be easy to remove the support in future (follow up PR)?
There was a problem hiding this comment.
I've removed it now, after thinking about it for a wee while forcing users to a particular representation for a non-native type felt like a bad idea. I've updated the code and added explicit mention of the decision in here: d9f0aee
| /// Indicates if an unwind may happen across a call to this function. | ||
| pub can_unwind: bool, | ||
| /// Computed type discriminator for pointer authentication purpose. | ||
| pub ptrauth_type_discriminator: u64, |
There was a problem hiding this comment.
@kovdan01 I'll answer you comments re FnAbi here, to keep things in one place.
So this boils down to two things: can we do it and should we do it?
Can we do it - yes, I had a go and it is pretty straight forward, please see the implementation.
As to the latter, it's a bit more complicated. The way I understand the pipeline is as follows:
- discriminator is a property of the source-level function type, much more aligned with
FnSig - whereas
FnAbiis a property of the lowered calling convention, how arguments to a function should be handled.
Those are intentionally different abstractions.
If FnAbi is used, we would forever have to rely on ArgAbi, which is not designed to preserve source-level type information. Although ArgAbi::layout.ty currently exists, it is only used in a very limited way (primarily for an "equal enough" comparison for vec types), while most uses of layout revolve around things like size of the type and it's layout, not the type logic per se. To me it suggests that the presence of the original type is incidental rather than a stable guarantee.
More importantly, the FnAbi documentation explicitly states that it may not match the original MIR function signature. Compiler transformations such as #[track_caller] and GPU offload can introduce (implicit) arguments that would not be present in a corresponding FnSig. While fixed_count distinguishes fixed and variadic arguments, it does not provide a way to recover the original source-level parameter list in the presence of these inserted arguments, or at least the comments does not give a strong enough guarantee that it would.
As to CanonAbi I think matching against CanonAbi::C should be good enough for all practical uses. And should we ever discover it is not, we could use the abi map hook that guards the conversion of ExternAbi to CanonAbi, so we would be able to control it fully.
To sum up, using FnAbi seems like layering violation, we would be trying to reconstruct information that does not belong to the llvm codegen stage from bits of data that does exist, but is not meant for that purpose.
There was a problem hiding this comment.
@jchlanda Thanks for the research and the prototype! I agree with your point - let's stick with current approach and just have the alternative just in case it's considered a better solution during proper review conducted by actual rust maintainers
697a1f4 to
c224581
Compare
Alongside Rust implementation of LLVM's SipHash-2-4.
This is only to be used when emitting an operand bundle, which might not have access to an Instance of the function, but for which FnAbi is guaranteed to be correct.
Start moving away from global schema, by cloning it into users. Helpers for fn ptr type discrimination status and value of the key.
This is a move away from a blind bitcast between the types. Now, when dealing with function pointers, transmute will detect a domain change (change of fn ptr type) and issue resigning.
As the libc changes was merged in to the main we do not need to patch it. cc-rs still needs to be patched as compiler/rustc_llvm/Cargo.toml still pins to an old version: cc = "=1.2.16"
…g get_fn_addr call sites
c224581 to
cff5996
Compare
5 participants
View all comments