[HWToLLVM] ArrayGet lowering produces out-of-bounds memory access

[fabianschuiki]

Array get/slice lowering patterns in HW-to-LLVM generate unguarded loads, potentially loading values from invalid memory locations:

circt/lib/Conversion/HWToLLVM/HWToLLVM.cpp

Lines 136 to 176 in ce9877c

	/// Convert an ArrayGetOp to the LLVM dialect.
	/// Pattern: array_get(input, index) =>
	/// load(gep(store(input, alloca), zext(index)))
	struct ArrayGetOpConversion : public ConvertOpToLLVMPattern<hw::ArrayGetOp> {
	using ConvertOpToLLVMPattern<hw::ArrayGetOp>::ConvertOpToLLVMPattern;
	LogicalResult
	matchAndRewrite(hw::ArrayGetOp op, OpAdaptor adaptor,
	ConversionPatternRewriter &rewriter) const override {
	Value arrPtr;
	if (auto load = adaptor.getInput().getDefiningOp<LLVM::LoadOp>()) {
	// In this case the array was loaded from an existing address, so we can
	// just grab that address instead of reallocating the array on the stack.
	arrPtr = load.getAddr();
	} else {
	auto oneC = rewriter.create<LLVM::ConstantOp>(
	op->getLoc(), IntegerType::get(rewriter.getContext(), 32),
	rewriter.getI32IntegerAttr(1));
	arrPtr = rewriter.create<LLVM::AllocaOp>(
	op->getLoc(), LLVM::LLVMPointerType::get(rewriter.getContext()),
	adaptor.getInput().getType(), oneC,
	/*alignment=*/4);
	rewriter.create<LLVM::StoreOp>(op->getLoc(), adaptor.getInput(), arrPtr);
	}
	auto arrTy = typeConverter->convertType(op.getInput().getType());
	auto elemTy = typeConverter->convertType(op.getResult().getType());
	auto zextIndex = zextByOne(op->getLoc(), rewriter, op.getIndex());
	// During the ongoing migration to opaque types, use the constructor that
	// accepts an element type when the array pointer type is opaque, and
	// otherwise use the typed pointer constructor.
	auto gep = rewriter.create<LLVM::GEPOp>(
	op->getLoc(), LLVM::LLVMPointerType::get(rewriter.getContext()), arrTy,
	arrPtr, ArrayRef<LLVM::GEPArg>{0, zextIndex});
	rewriter.replaceOpWithNewOp<LLVM::LoadOp>(op, elemTy, gep);
	return success();
	}
	};

In case the index is not a power of two, we should probably add an scf.if here to only perform the load if it is within the range of the array.