-
Notifications
You must be signed in to change notification settings - Fork 15k
Add deactivation symbol operand to ConstantPtrAuth. #133537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: users/pcc/spr/main.add-deactivation-symbol-operand-to-constantptrauth
Are you sure you want to change the base?
Changes from 16 commits
e471928
90ec548
3190e38
150a83f
4042701
8413f5e
f4981a9
e728f34
063c7a4
cff37c4
60e836e
a780d18
51c353b
fae818f
f0afa10
1656b78
4890d9a
cb4704b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1609,7 +1609,16 @@ Expected<Value *> BitcodeReader::materializeValue(unsigned StartValID, | |
| if (!Disc) | ||
| return error("ptrauth disc operand must be ConstantInt"); | ||
|
|
||
| C = ConstantPtrAuth::get(ConstOps[0], Key, Disc, ConstOps[3]); | ||
| Constant *DeactivationSymbol = | ||
| ConstOps.size() > 4 ? ConstOps[4] | ||
| : ConstantPointerNull::get(cast<PointerType>( | ||
| ConstOps[3]->getType())); | ||
| if (!DeactivationSymbol->getType()->isPointerTy()) | ||
| return error( | ||
| "ptrauth deactivation symbol operand must be a pointer"); | ||
|
|
||
| C = ConstantPtrAuth::get(ConstOps[0], Key, Disc, ConstOps[3], | ||
| DeactivationSymbol); | ||
| break; | ||
| } | ||
| case BitcodeConstant::NoCFIOpcode: { | ||
|
|
@@ -3809,6 +3818,16 @@ Error BitcodeReader::parseConstants() { | |
| (unsigned)Record[2], (unsigned)Record[3]}); | ||
| break; | ||
| } | ||
| case bitc::CST_CODE_PTRAUTH2: { | ||
| if (Record.size() < 4) | ||
|
||
| return error("Invalid ptrauth record"); | ||
| // Ptr, Key, Disc, AddrDisc, DeactivationSymbol | ||
| V = BitcodeConstant::create( | ||
| Alloc, CurTy, BitcodeConstant::ConstantPtrAuthOpcode, | ||
| {(unsigned)Record[0], (unsigned)Record[1], (unsigned)Record[2], | ||
| (unsigned)Record[3], (unsigned)Record[4]}); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| assert(V->getType() == getTypeByID(CurTyID) && "Incorrect result type ID"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3007,11 +3007,12 @@ void ModuleBitcodeWriter::writeConstants(unsigned FirstVal, unsigned LastVal, | |
| Record.push_back(VE.getTypeID(NC->getGlobalValue()->getType())); | ||
| Record.push_back(VE.getValueID(NC->getGlobalValue())); | ||
| } else if (const auto *CPA = dyn_cast<ConstantPtrAuth>(C)) { | ||
| Code = bitc::CST_CODE_PTRAUTH; | ||
| Code = bitc::CST_CODE_PTRAUTH2; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be Constant *DeactivationSymbol = CPA->getDeactivationSymbol();
Code = DeactivationSymbol->isNullValue() ? CST_CODE_PTRAUTH : CST_CODE_PTRAUTH2;
...
if (!DeactivationSymbol->isNullValue())
Record.push_back(VE.getValueID(DeactivationSymbol));There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated to correct the of the Code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bitcode doesn't have forwards compatibility, only backwards compatibility. So I think it's fine to use PTRAUTH2 unconditionally here. |
||
| Record.push_back(VE.getValueID(CPA->getPointer())); | ||
| Record.push_back(VE.getValueID(CPA->getKey())); | ||
| Record.push_back(VE.getValueID(CPA->getDiscriminator())); | ||
| Record.push_back(VE.getValueID(CPA->getAddrDiscriminator())); | ||
| Record.push_back(VE.getValueID(CPA->getDeactivationSymbol())); | ||
| } else { | ||
| #ifndef NDEBUG | ||
| C->dump(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1699,7 +1699,9 @@ LLVMValueRef LLVMConstantPtrAuth(LLVMValueRef Ptr, LLVMValueRef Key, | |
| LLVMValueRef Disc, LLVMValueRef AddrDisc) { | ||
| return wrap(ConstantPtrAuth::get( | ||
| unwrap<Constant>(Ptr), unwrap<ConstantInt>(Key), | ||
| unwrap<ConstantInt>(Disc), unwrap<Constant>(AddrDisc))); | ||
| unwrap<ConstantInt>(Disc), unwrap<Constant>(AddrDisc), | ||
| ConstantPointerNull::get( | ||
| cast<PointerType>(unwrap<Constant>(AddrDisc)->getType())))); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to extend the C API to give access to this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reckon that could be done in a followup if anyone needs it. |
||
| } | ||
|
|
||
| /*-- Opcode mapping */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -2631,6 +2631,14 @@ void Verifier::visitConstantPtrAuth(const ConstantPtrAuth *CPA) { | |||
|
|
||||
| Check(CPA->getDiscriminator()->getBitWidth() == 64, | ||||
| "signed ptrauth constant discriminator must be i64 constant integer"); | ||||
|
|
||||
| Check(CPA->getDeactivationSymbol()->getType()->isPointerTy(), | ||||
| "signed ptrauth constant deactivation symbol must be a pointer"); | ||||
|
|
||||
| Check(isa<GlobalValue>(CPA->getDeactivationSymbol()) || | ||||
| CPA->getDeactivationSymbol()->isNullValue(), | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe also check
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's probably best to check that in the I had already added a check for this operand being a pointer in the .ll parser; I noticed that there was no test coverage for that so I added it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For things we can easily check in the verifier, I prefer to check them even if there's also an assertion, to catch issues in assert-disabled builds. Also, missing check in the bitcode parser. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Isn't the verifier disabled by default in no-asserts builds? So I guess it wouldn't make much of a difference.
(Looks like the comments were meant to read "no-asserts", I sent #157769 for that.) That being said I don't feel strongly so I added it.
Added. |
||||
| "signed ptrauth constant deactivation symbol must be a global value " | ||||
| "or null"); | ||||
| } | ||||
|
|
||||
| bool Verifier::verifyAttributeCount(AttributeList Attrs, unsigned Params) { | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to do this here, but we probably should make the optional operands (in textual IR) optional here as well, and implicitly make them null? Now that I think about it, I'm not sure how idiomatic that would be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it, but since there are only a few places that need this, it seemed slightly better to be explicit about all the operands since that's consistent with what we have elsewhere.
What might be nice is if we initialized this using fields of a passed-in struct so that call sites are more readable, but that's a separate change.