Skip to content

Commit a1441b2

Browse files
authored
Add additional guardrails against unsigned underflows caused by wrong WordCount (#3787)
Following up on #3671, this adds additional guardrails for cases that were still unhandled by the previous patch. This PR also includes multiple new tests for invalid wordcounts, and joins them all --the new, and the ones added by #3671-- in their own directory (test/negative/invalid-wordcount).
1 parent 7d75932 commit a1441b2

37 files changed

Lines changed: 307 additions & 0 deletions

lib/SPIRV/libSPIRV/SPIRVDecorate.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ class SPIRVDecorate : public SPIRVDecorateGeneric {
125125
public:
126126
static const Op OC = OpDecorate;
127127
static const SPIRVWord FixedWC = 3;
128+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
128129
// Complete constructor for decorations without literals
129130
SPIRVDecorate(Decoration TheDec, SPIRVEntry *TheTarget)
130131
: SPIRVDecorateGeneric(OC, 3, TheDec, TheTarget) {}
@@ -240,6 +241,7 @@ class SPIRVDecorateId : public SPIRVDecorateGeneric {
240241
public:
241242
static const Op OC = OpDecorateId;
242243
static const SPIRVWord FixedWC = 3;
244+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
243245
// Complete constructor for decorations with one id operand
244246
SPIRVDecorateId(Decoration TheDec, SPIRVEntry *TheTarget, SPIRVId V)
245247
: SPIRVDecorateGeneric(OC, 4, TheDec, TheTarget, V) {}
@@ -330,6 +332,7 @@ class SPIRVMemberDecorate : public SPIRVDecorateGeneric {
330332
public:
331333
static const Op OC = OpMemberDecorate;
332334
static const SPIRVWord FixedWC = 4;
335+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
333336
// Complete constructor for decorations without literals
334337
SPIRVMemberDecorate(Decoration TheDec, SPIRVWord Member,
335338
SPIRVEntry *TheTarget)
@@ -447,6 +450,8 @@ class SPIRVGroupDecorateGeneric : public SPIRVEntryNoIdGeneric {
447450
SPIRVCK(WordCount >= FixedWC, InvalidWordCount, "");
448451
Targets.resize(WordCount - FixedWC);
449452
}
453+
454+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
450455
virtual void decorateTargets() = 0;
451456
_SPIRV_DCL_ENCDEC
452457
protected:

lib/SPIRV/libSPIRV/SPIRVEntry.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -380,6 +380,10 @@ class SPIRVEntry {
380380
/// its variable sized member before decoding the remaining words.
381381
virtual void setWordCount(SPIRVWord TheWordCount);
382382

383+
/// Return the minimum valid WordCount (1, the OpCode). Other entries will
384+
/// override to return their respective fixed word counts.
385+
virtual SPIRVWord getFixedWordCount() const { return 1; }
386+
383387
/// Create an empty SPIRV object by op code, e.g. OpTypeInt creates
384388
/// SPIRVTypeInt.
385389
static SPIRVEntry *create(Op);
@@ -541,6 +545,7 @@ class SPIRVAnnotation : public SPIRVAnnotationGeneric {
541545
class SPIRVEntryPoint : public SPIRVAnnotation {
542546
public:
543547
static const SPIRVWord FixedWC = 4;
548+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
544549
SPIRVEntryPoint(SPIRVModule *TheModule, SPIRVExecutionModelKind,
545550
SPIRVId TheId, const std::string &TheName,
546551
std::vector<SPIRVId> Variables);

lib/SPIRV/libSPIRV/SPIRVFnVar.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ bool specializeFnVariants(SPIRVModule *BM, std::string &ErrMsg);
5151
class SPIRVConditionalEntryPointINTEL : public SPIRVAnnotation {
5252
public:
5353
static const SPIRVWord FixedWC = 5;
54+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
5455
SPIRVConditionalEntryPointINTEL(SPIRVModule *TheModule, SPIRVId Condition,
5556
SPIRVExecutionModelKind TheExecModel,
5657
SPIRVId TheId, const std::string &TheName,
@@ -249,6 +250,7 @@ class SPIRVSpecConstantTargetINTEL : public SPIRVValue {
249250

250251
return Res;
251252
}
253+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
252254

253255
protected:
254256
_SPIRV_DEF_ENCDEC4(Type, Id, Target, Features);
@@ -414,6 +416,7 @@ class SPIRVSpecConstantCapabilitiesINTEL : public SPIRVValue {
414416

415417
return Res;
416418
}
419+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
417420

418421
protected:
419422
_SPIRV_DEF_ENCDEC3(Type, Id, Capabilities);

lib/SPIRV/libSPIRV/SPIRVInstruction.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,8 +280,14 @@ class SPIRVInstTemplateBase : public SPIRVInstruction {
280280
// instructions.
281281
updateModuleVersion();
282282
}
283+
SPIRVWord getFixedWordCount() const override {
284+
// OpCode word plus the optional result-type and
285+
// result-id words. Operands beyond this are variable length.
286+
return 1 + (hasType() ? 1 : 0) + (hasId() ? 1 : 0);
287+
}
283288
void setWordCount(SPIRVWord TheWordCount) override {
284289
SPIRVEntry::setWordCount(TheWordCount);
290+
SPIRVCK(WordCount >= getFixedWordCount(), InvalidWordCount, "");
285291
auto NumOps = WordCount - 1;
286292
if (hasId())
287293
--NumOps;
@@ -512,6 +518,7 @@ class SPIRVVariableBase : public SPIRVInstruction {
512518
return std::vector<SPIRVEntry *>(1, V);
513519
return std::vector<SPIRVEntry *>();
514520
}
521+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
515522

516523
protected:
517524
void validate() const override {

lib/SPIRV/libSPIRV/SPIRVModule.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2484,6 +2484,15 @@ static SPIRVEntry *parseAndCreateSPIRVEntry(SPIRVWord &WordCount, Op &OpCode,
24842484
if (Scope && !isModuleScopeAllowedOpCode(OpCode)) {
24852485
Entry->setScope(Scope);
24862486
}
2487+
// Reject a WordCount below the instruction's minimum before setWordCount().
2488+
if (!M.getErrorLog().checkError(WordCount >= Entry->getFixedWordCount(),
2489+
SPIRVEC_InvalidWordCount,
2490+
"WordCount is smaller than the instruction's "
2491+
"minimum word count")) {
2492+
M.setInvalid();
2493+
delete Entry;
2494+
return nullptr;
2495+
}
24872496
Entry->setWordCount(WordCount);
24882497
if (OpCode != OpLine)
24892498
Entry->setLine(M.getCurrentLine());

lib/SPIRV/libSPIRV/SPIRVType.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -660,6 +660,7 @@ class SPIRVTypeImage : public SPIRVType {
660660
std::vector<SPIRVEntry *> getNonLiteralOperands() const override {
661661
return std::vector<SPIRVEntry *>(1, get<SPIRVType>(SampledType));
662662
}
663+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
663664

664665
protected:
665666
_SPIRV_DEF_ENCDEC9(Id, SampledType, Desc.Dim, Desc.Depth, Desc.Arrayed,
@@ -805,6 +806,8 @@ class SPIRVTypeStruct : public SPIRVType {
805806
MemberTypeIdVec.resize(WordCount - FixedWC);
806807
}
807808

809+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
810+
808811
// TODO: Should we attach operands of continued instructions as well?
809812
std::vector<SPIRVEntry *> getNonLiteralOperands() const override {
810813
std::vector<SPIRVEntry *> Operands(MemberTypeIdVec.size());
@@ -877,6 +880,7 @@ class SPIRVTypeFunction : public SPIRVType {
877880
Operands.push_back(getEntry(I));
878881
return Operands;
879882
}
883+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
880884

881885
protected:
882886
_SPIRV_DEF_ENCDEC3(Id, ReturnType, ParamTypeIdVec)

lib/SPIRV/libSPIRV/SPIRVValue.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ template <spv::Op OC> class SPIRVConstantBase : public SPIRVValue {
174174
double getDoubleValue() const { return getValue<double>(); }
175175
unsigned getNumWords() const { return NumWords; }
176176
const std::vector<SPIRVWord> &getSPIRVWords() { return Words; }
177+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
177178

178179
protected:
179180
constexpr static SPIRVWord FixedWC = 3;
@@ -210,6 +211,7 @@ template <spv::Op OC> class SPIRVConstantBase : public SPIRVValue {
210211
}
211212
void setWordCount(SPIRVWord WordCount) override {
212213
SPIRVValue::setWordCount(WordCount);
214+
SPIRVCK(WordCount >= FixedWC, InvalidWordCount, "");
213215
NumWords = WordCount - FixedWC;
214216
}
215217
void decode(std::istream &I) override {
@@ -358,6 +360,7 @@ template <spv::Op OC> class SPIRVConstantCompositeBase : public SPIRVValue {
358360
for (auto &I : ContinuedInstructions)
359361
O << *I;
360362
}
363+
SPIRVWord getFixedWordCount() const override { return FixedWC; }
361364

362365
protected:
363366
void validate() const override {
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
; OpAliasScopeDeclINTEL minimum = 2 (header + result-id); the scope arguments
2+
; are variable length. WordCount=1 is malformed.
3+
; Test that malformed input is rejected early rather than silently causing
4+
; integer underflow and potential process hang.
5+
6+
; RUN: not llvm-spirv %s -to-binary -o %t.spv 2>&1 | FileCheck %s
7+
; CHECK: InvalidWordCount:
8+
9+
119734787 65536 0 10 0
10+
2 Capability Kernel
11+
3 MemoryModel 1 2
12+
1 AliasScopeDeclINTEL 1
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
; OpAsmCallINTEL minimum = 4 (header + result-type + result-id + asm-target);
2+
; the arguments are variable length. WordCount=3 is malformed.
3+
; Test that malformed input is rejected early rather than silently causing
4+
; integer underflow and potential process hang.
5+
6+
; RUN: not llvm-spirv %s -to-binary -o %t.spv 2>&1 | FileCheck %s
7+
; CHECK: InvalidWordCount:
8+
9+
119734787 65536 0 10 0
10+
2 Capability Kernel
11+
3 MemoryModel 1 2
12+
3 AsmCallINTEL 1 2 3

test/negative/invalid-wordcount-branchconditional.spt renamed to test/negative/invalid-wordcount/invalid-wordcount-branchconditional.spt

File renamed without changes.

0 commit comments

Comments
 (0)