From d5bdd07c01188f053f1d20127aab0bcb485da636 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Fri, 18 Jun 2021 13:14:38 +0100 Subject: [PATCH] Fix out of bounds index into VlWide under AstSel When part selecting bits via an AstSel in a VlWide, V3Expand used to do something akin to: word_index = lsb / 32; bit_index = lsb % 32; result = wide[word_index + 1] << (32 - bit_index) | wide[word_index] >> bit_index; The unconditional "+ 1" can cause an out of bounds access into the VlWide, when the whole of the select is into the most significant word (i.e.: when word_index is already the most significant word). We now emit roughly this instead: lo_word_index = lsb / 32; bit_index = lsb % 32; hi_word_index = (lsb + width - 1) / 32; result = wide[hi_word_index] << (32 - bit_index) | wide[lo_word_index] >> bit_index; i.e.: we explicitly calculate which word the MSB of the select falls into, and address that word, rather than the unconditional + 1. The shifts ensure we still yield the right result, even if lo_word_index and hi_word_index are the same. Note: The actual expression created by V3Expand can be a bit more complicated as we might need to access 3 words when the result is a QData, all 3 word indices are calculated explicitly. --- src/V3Expand.cpp | 82 ++++++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/src/V3Expand.cpp b/src/V3Expand.cpp index dd5a5ec42..2f33136f7 100644 --- a/src/V3Expand.cpp +++ b/src/V3Expand.cpp @@ -344,6 +344,7 @@ private: // See under ASSIGN(WIDE) } else if (nodep->fromp()->isWide()) { UINFO(8, " SEL(wide) " << nodep << endl); + UASSERT_OBJ(nodep->widthConst() <= 64, nodep, "Inconsistent width"); // Selection amounts // Check for constant shifts & save some constification work later. // Grab lowest bit(s) @@ -358,60 +359,65 @@ private: AstNode* midp = nullptr; V3Number zero(nodep, longOrQuadWidth(nodep)); if (nodep->widthConst() > 1) { - AstNode* midwordp = // SEL(from,[1+wordnum]) + const uint32_t midMsbOffset + = std::min(nodep->widthConst(), VL_EDATASIZE) - 1; + AstNode* const midMsbp + = new AstAdd(nodep->lsbp()->fileline(), + new AstConst(nodep->lsbp()->fileline(), midMsbOffset), + nodep->lsbp()->cloneTree(false)); + AstNode* midwordp = // SEL(from,[midwordnum]) newWordSel(nodep->fromp()->fileline(), nodep->fromp()->cloneTree(true), - nodep->lsbp(), 1); + midMsbp, 0); + // newWordSel clones the index, so delete it + VL_DO_DANGLING(midMsbp->deleteTree(), midMsbp); if (nodep->isQuad() && !midwordp->isQuad()) { midwordp = new AstCCast(nodep->fileline(), midwordp, nodep); } - // If we're selecting bit zero, then all 32 bits in word 1 - // get shifted << by 32 bits - // else we need to form the lower word, so we << by 31 or less - // nbitsfromlow <= (lsb==0) ? 64-bitbit(lsb) : 32-bitbit(lsb) - AstNode* midshiftp + AstNode* const midshiftp = new AstSub(nodep->lsbp()->fileline(), new AstConst(nodep->lsbp()->fileline(), VL_EDATASIZE), newSelBitBit(nodep->lsbp())); - if (nodep->isQuad()) { - midshiftp = new AstCond( - nodep->fileline(), - new AstEq(nodep->fileline(), new AstConst(nodep->fileline(), 0), - newSelBitBit(nodep->lsbp())), - new AstConst(nodep->lsbp()->fileline(), VL_EDATASIZE), midshiftp); - } - AstNode* midmayp - = new AstShiftL(nodep->fileline(), midwordp, midshiftp, nodep->width()); - if (nodep->isQuad()) { - midp = midmayp; // Always grab from two words - } else { - midp = new AstCond(nodep->fileline(), - new AstEq(nodep->fileline(), - new AstConst(nodep->fileline(), 0), - newSelBitBit(nodep->lsbp())), - new AstConst(nodep->fileline(), zero), midmayp); - } + // If we're selecting bit zero, then all 32 bits in the mid word + // get shifted << by 32 bits, so ignore them. + midp = new AstCond( + nodep->fileline(), + // lsb % VL_EDATASIZE == 0 ? + + new AstEq(nodep->fileline(), new AstConst(nodep->fileline(), 0), + newSelBitBit(nodep->lsbp())), + // 0 : + new AstConst(nodep->fileline(), zero), + // midword >> (VL_EDATASIZE - (lbs % VL_EDATASIZE)) + new AstShiftL(nodep->fileline(), midwordp, midshiftp, nodep->width())); } // If > 32 bits, we might be crossing the second word boundary AstNode* hip = nullptr; if (nodep->widthConst() > VL_EDATASIZE) { - AstNode* hiwordp = // SEL(from,[2+wordnum]) - newWordSel(nodep->fromp()->fileline(), nodep->fromp()->cloneTree(true), - nodep->lsbp(), 2); + const uint32_t hiMsbOffset = nodep->widthConst() - 1; + AstNode* const hiMsbp + = new AstAdd(nodep->lsbp()->fileline(), + new AstConst(nodep->lsbp()->fileline(), hiMsbOffset), + nodep->lsbp()->cloneTree(false)); + AstNode* hiwordp = // SEL(from,[hiwordnum]) + newWordSel(nodep->fromp()->fileline(), nodep->fromp()->cloneTree(true), hiMsbp, + 0); + // newWordSel clones the index, so delete it + VL_DO_DANGLING(hiMsbp->deleteTree(), hiMsbp); if (nodep->isQuad() && !hiwordp->isQuad()) { hiwordp = new AstCCast(nodep->fileline(), hiwordp, nodep); } - AstNode* himayp - = new AstShiftL(nodep->fileline(), hiwordp, - // nbitsfromlow_and_mid <= 64-bitbit(lsb) - new AstSub(nodep->lsbp()->fileline(), - new AstConst(nodep->lsbp()->fileline(), 64), - newSelBitBit(nodep->lsbp())), - nodep->width()); - // if (frombit==0) then ignore, else use it - hip = new AstCond(nodep->fileline(), + AstNode* const hishiftp + = new AstCond(nodep->fileline(), + // lsb % VL_EDATASIZE == 0 ? new AstEq(nodep->fileline(), new AstConst(nodep->fileline(), 0), newSelBitBit(nodep->lsbp())), - new AstConst(nodep->fileline(), zero), himayp); + // VL_EDATASIZE : + new AstConst(nodep->lsbp()->fileline(), VL_EDATASIZE), + // 64 - (lbs % VL_EDATASIZE) + new AstSub(nodep->lsbp()->fileline(), + new AstConst(nodep->lsbp()->fileline(), 64), + newSelBitBit(nodep->lsbp()))); + hip = new AstShiftL(nodep->fileline(), hiwordp, hishiftp, nodep->width()); } AstNode* newp = lowp;