Changeset View
Standalone View
llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp
| Show First 20 Lines • Show All 3,143 Lines • ▼ Show 20 Lines | bool X86AsmParser::validateInstruction(MCInst &Inst, const OperandVector &Ops) { | ||||
| } | } | ||||
| } | } | ||||
| return false; | return false; | ||||
| } | } | ||||
| static const char *getSubtargetFeatureName(uint64_t Val); | static const char *getSubtargetFeatureName(uint64_t Val); | ||||
| void X86AsmParser::emitInstruction(MCInst &Inst, OperandVector &Operands, | void X86AsmParser::emitInstruction(MCInst &Inst, OperandVector &Operands, | ||||
craig.topper: Move the command line option to the top of the file below the includes.
The two other examples… | |||||
| MCStreamer &Out) { | MCStreamer &Out) { | ||||
| Out.emitInstruction(Inst, getSTI()); | Out.emitInstruction(Inst, getSTI()); | ||||
Not Done ReplyInline ActionsLet's add a comment about how this list was created, or maybe a reference to public documentation if this list is pulled from there. Otherwise folks coming afterward have no way to tell if this list is correct. mattdr: Let's add a comment about how this list was created, or maybe a reference to public… | |||||
| if (getSTI().getFeatureBits()[X86::FeatureLVILoadHardening]) { | |||||
craig.topperUnsubmitted Not Done ReplyInline ActionsCan we move this code out to a separate method called from here? Something like performLVIMitigation? craig.topper: Can we move this code out to a separate method called from here? Something like… | |||||
sconstabAuthorUnsubmitted I created two new methods. Let me know what you think. sconstab: I created two new methods. Let me know what you think. | |||||
| auto Flags = Inst.getFlags(); | |||||
| if ((Flags & X86::REP_PREFIX) || (Flags & X86::REPNE_PREFIX)) { | |||||
| switch (Inst.getOpcode()) { | |||||
| case X86::CMPSB: | |||||
| case X86::CMPSW: | |||||
| case X86::CMPSL: | |||||
| case X86::CMPSQ: | |||||
Not Done ReplyInline ActionsFollowing up on my previous comment: it seems like this is supposed to be the full set of instructions that combine a load from memory with a control flow change dependent on that load. How are we sure the list is complete? Just from a quick look at the return instructions LLVM has for X86, it looks like LRETL and LRETQ exist and would not be mitigated. Can we just look at the MCInstDesc and warn for instructions that mayLoad and mayAffectControlFlow? Then this list will stay up to date even if new instructions are added. mattdr: Following up on my previous comment: it seems like this is supposed to be the full set of… | |||||
Not Done ReplyInline ActionsI looked into this and unfortunately the mayLoad attribute is not set for any of the RET family (see https://github.com/llvm/llvm-project/blob/b1d581019f5d4d176ad70ddffee13db247a13ef1/llvm/lib/Target/X86/X86InstrControl.td#L21). Either this is a bug or I don't actually understand what the mayLoad attribute actually means. It seems pretty strange since the POPr family has mayLoad. @craig.topper : opinion? sconstab: I looked into this and unfortunately the mayLoad attribute is not set for any of the RET family… | |||||
Not Done ReplyInline ActionsI suspect it doesn't have the mayLoad flag because from the perspective of the codegen passes that normally use that flag, the load of the return address isn't really something we would care about. It could just as well be reading the return address from some stack dedicated return address stack or a stack implemented in hardware or something. Not sure what the implications of just adding the flag would be to codegen. craig.topper: I suspect it doesn't have the mayLoad flag because from the perspective of the codegen passes… | |||||
Not Done ReplyInline ActionsYikes, okay. It's incredibly surprising that mayLoad isn't set for the RET family, and if anything stops working when we set it that's almost certainly a bug we want to know about. But I can accept that fixing that is out of scope for this change -- at least, for the initial version. If so, though, that makes it all the more important that we describe how we arrived at this particular list of instructions, in enough detail that someone else can retrace our steps. mattdr: Yikes, okay. It's incredibly surprising that `mayLoad` isn't set for the `RET` family, and if… | |||||
| case X86::SCASB: | |||||
| case X86::SCASW: | |||||
| case X86::SCASL: | |||||
| case X86::SCASQ: | |||||
| Warning(Inst.getLoc(), "Instruction may be vulnerable to LVI and " | |||||
| "requires manual mitigation"); | |||||
| return; | |||||
| } | |||||
| } | |||||
| } | |||||
| if (getSTI().getFeatureBits()[X86::FeatureLVIControlFlowIntegrity]) { | |||||
| switch (Inst.getOpcode()) { | |||||
| case X86::RET: | |||||
| case X86::RETL: | |||||
| case X86::RETQ: | |||||
| case X86::RETIL: | |||||
Not Done ReplyInline ActionsRET is a pseudo. It will never be parsed. craig.topper: RET is a pseudo. It will never be parsed.
RETW is missing.
LRETL, LRETW, LRETQ, LRETIL, LRETIW… | |||||
| case X86::RETIQ: | |||||
| case X86::RETIW: | |||||
Not Done ReplyInline ActionsOnce the details of LVI have faded, future maintainers will really appreciate a comment here about what this mitigation *is*, what it's doing, etc. mattdr: Once the details of LVI have faded, future maintainers will really appreciate a comment here… | |||||
Not Done ReplyInline ActionsThis warning should at least tell users *why* this is vulnerable, and ideally offer the next step for fixing it. mattdr: This warning should at least tell users *why* this is vulnerable, and ideally offer the next… | |||||
Not Done ReplyInline ActionsThe manual mitigation is a little bit more involved than what would typically be conveyed in a compiler warning. Would it be acceptable for the warning to point to the LVI deep dive document? sconstab: The manual mitigation is a little bit more involved than what would typically be conveyed in a… | |||||
Not Done ReplyInline ActionsSure, a link to an external site seems fine. Can we please link to the specific part of the page that explains the mitigations for this specific set of instructions? mattdr: Sure, a link to an external site seems fine. Can we please link to the specific part of the… | |||||
@mattdr Working on it! sconstab: @mattdr Working on it! | |||||
| case X86::JMP16m: | |||||
| case X86::JMP32m: | |||||
| case X86::JMP64m: | |||||
| case X86::JMP64m_REX: | |||||
| case X86::FARJMP16m: | |||||
| case X86::FARJMP32m: | |||||
Not Done ReplyInline ActionsSame comment as for the list above. mattdr: Same comment as for the list above. | |||||
| case X86::FARJMP64: | |||||
Not Done ReplyInline ActionsJMP64m_REX is a special codegen pseudo instruction. It won't be parsed. craig.topper: JMP64m_REX is a special codegen pseudo instruction. It won't be parsed. | |||||
| case X86::CALL16m: | |||||
Not Done ReplyInline Actionsbinutils does not handle the FARJMP/FARCALL. Is that a miss or them or something else? craig.topper: binutils does not handle the FARJMP/FARCALL. Is that a miss or them or something else? | |||||
| case X86::CALL32m: | |||||
| case X86::CALL64m: | |||||
| case X86::FARCALL16m: | |||||
| case X86::FARCALL32m: | |||||
| case X86::FARCALL64: | |||||
| Warning(Inst.getLoc(), "Instruction may be vulnerable to LVI and " | |||||
| "requires manual mitigation"); | |||||
| return; | |||||
| } | |||||
| } | |||||
Not Done ReplyInline ActionsI don't think this works if the rep prefix is on the line before the cmps/scas instruction. That will assemble to the same thing but the parser sees it as two instructions. craig.topper: I don't think this works if the rep prefix is on the line before the cmps/scas instruction. | |||||
Not Done ReplyInline Actions@craig.topper any notion what the best workaround would be? One option is keeping track of whether we've seen a REP or REPNE before this. Although perhaps a simpler approach is just not worrying about whether we saw REP. We're here to try to warn users about instructions that we can't mitigate. What's the likelihood that we see a CMPSB without REP and it turns out it _doesn't_ need mitigation? mattdr: @craig.topper any notion what the best workaround would be? One option is keeping track of… | |||||
@mattdr The difference is that CMPSB without REP can be mitigated simply by following it with an LFENCE. REP CMPSB must be manually decomposed into a loop, into which an LFENCE can be inserted. I wonder if it would suffice to simply emit a warning/error if a lone REP is encountered to indicate that the user may need to manually mitigate? sconstab: @mattdr The difference is that `CMPSB` without `REP` can be mitigated simply by following it… | |||||
| // If this instruction loads and we're hardening for LVI, emit an LFENCE. | |||||
Not Done ReplyInline ActionsI didn't find any mention of these instructions, why they would "require additional considerations", or what folks should actually *do* if they hit this warning by reading https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection. Here's my best guess: Is that accurate? If so, we could explain it that way and replace the hardcoded list with hasImplicitDefOfPhysReg(X86::EFLAGS) mattdr: I didn't find any mention of these instructions, why they would "require additional… | |||||
Not Done ReplyInline ActionsAnd if we hit the same problem as above with RET, where somehow the instruction tables are inaccurate and we can't rely on them yet, then it's similarly okay with me if we leave the hardcoded list of instructions as long as we provide really specific comments describing why the list is all and only these instructions. mattdr: And if we hit the same problem as above with `RET`, where somehow the instruction tables are… | |||||
Similar to providing a link to a public document in a warning message, I think it should suffice to have a link to said document (and specifically the relevant subsection of that document) in the comment. sconstab: Similar to providing a link to a public document in a warning message, I think it should… | |||||
| if (getSTI().getFeatureBits()[X86::FeatureLVILoadHardening]) { | |||||
| const MCInstrDesc &MCID = MII.get(Inst.getOpcode()); | |||||
| // LFENCE has the mayLoad property, don't double fence. | |||||
| if (MCID.mayLoad() && Inst.getOpcode() != X86::LFENCE) { | |||||
| MCInst FenceInst; | |||||
| FenceInst.setOpcode(X86::LFENCE); | |||||
| FenceInst.setLoc(Inst.getLoc()); | |||||
| Out.emitInstruction(FenceInst, getSTI()); | |||||
| } | |||||
| } | |||||
| } | } | ||||
| bool X86AsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode, | bool X86AsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode, | ||||
| OperandVector &Operands, | OperandVector &Operands, | ||||
| MCStreamer &Out, uint64_t &ErrorInfo, | MCStreamer &Out, uint64_t &ErrorInfo, | ||||
| bool MatchingInlineAsm) { | bool MatchingInlineAsm) { | ||||
| if (isParsingIntelSyntax()) | if (isParsingIntelSyntax()) | ||||
| return MatchAndEmitIntelInstruction(IDLoc, Opcode, Operands, Out, ErrorInfo, | return MatchAndEmitIntelInstruction(IDLoc, Opcode, Operands, Out, ErrorInfo, | ||||
| ▲ Show 20 Lines • Show All 841 Lines • Show Last 20 Lines | |||||
Move the command line option to the top of the file below the includes.
The two other examples I know of start with "-x86-experimental-" instead of having experimental at the end.