diff --git a/.github/codeql-cpp.yml b/.github/codeql-cpp.yml index d99cdb559244..240c804e9fb7 100644 --- a/.github/codeql-cpp.yml +++ b/.github/codeql-cpp.yml @@ -3,3 +3,4 @@ name: "Custom CodeQL Analysis" queries: - uses: ./.github/codeql/custom-queries/cpp/deprecatedFunctionUsage.ql - uses: ./.github/codeql/custom-queries/cpp/dslDatasetHoldReleMismatch.ql + - uses: ./.github/codeql/custom-queries/cpp/zilFamByteswap.ql diff --git a/.github/codeql/custom-queries/cpp/zilFamByteswap.ql b/.github/codeql/custom-queries/cpp/zilFamByteswap.ql new file mode 100644 index 000000000000..416547019c56 --- /dev/null +++ b/.github/codeql/custom-queries/cpp/zilFamByteswap.ql @@ -0,0 +1,154 @@ +/** + * @name ZIL replay flexible array left un-byteswapped + * @description Detects byteswap_uint*_array(p, sizeof(*p)) / sizeof(T) on a + * log-record type that ends in a multi-byte flexible array member + * (e.g. blkptr_t lr_bps[]). sizeof() only covers the fixed header, + * so the trailing FAM stays foreign-endian on opposite-endian ZIL + * replay. Opaque uint8_t[] FAMs (names, raw write data) are ignored. + * See TX_CLONE_RANGE replay (zfs_replay_clone_range / + * zvol_replay_clone_range). + * @kind problem + * @severity error + * @tags correctness + * endianness + * @id cpp/zilFamByteswap + */ + +import cpp + +/** + * A struct that ends with a C99 flexible array member or a GCC zero-length + * array used the same way. + */ +class FlexibleArrayStruct extends Struct { + Field fam; + + FlexibleArrayStruct() { + fam = this.getAField() and + ( + ( + fam.getType() instanceof ArrayType and + not fam.getType().(ArrayType).hasArraySize() + ) + or + ( + fam.getType() instanceof ArrayType and + fam.getType().(ArrayType).hasArraySize() and + fam.getType().(ArrayType).getArraySize() = 0 + ) + ) and + not exists(Field f2 | + f2 = this.getAField() and f2.getByteOffset() > fam.getByteOffset() + ) + } + + Field getFam() { result = fam } + + Type getFamElementType() { + result = fam.getType().(ArrayType).getBaseType().getUnspecifiedType() + } + + /** Prefer typedef name (lr_clone_range_t) over the tag name. */ + string getDisplayName() { + exists(TypedefType t | + t.getBaseType().getUnspecifiedType() = this and result = t.getName() + ) + or + ( + not exists(TypedefType t | t.getBaseType().getUnspecifiedType() = this) and + this.getName() != "" and + result = this.getName() + ) + or + ( + not exists(TypedefType t | t.getBaseType().getUnspecifiedType() = this) and + this.getName() = "" and + result = "" + ) + } + + /** + * FAM holds multi-byte structured data that a header-only sizeof() byteswap + * would leave wrong-endian. + */ + predicate hasStructuredFam() { + exists(Type et | et = this.getFamElementType() | et.getSize() > 1) + } +} + +class ByteswapCall extends FunctionCall { + ByteswapCall() { + this.getTarget().getName().regexpMatch("byteswap_uint(64|32|16)_array") + } + + Expr getBufferArg() { result = this.getArgument(0) } + + Expr getSizeArg() { result = this.getArgument(1) } +} + +Type strip(Type t) { result = t.getUnspecifiedType() } + +predicate typeIsFlexibleStruct(Type t, FlexibleArrayStruct fas) { + strip(t) = fas + or + strip(t).(PointerType).getBaseType().getUnspecifiedType() = fas + or + exists(TypedefType ta | + strip(t) = ta and strip(ta.getBaseType()) = fas + ) + or + exists(TypedefType ta | + strip(t).(PointerType).getBaseType().getUnspecifiedType() = ta and + strip(ta.getBaseType()) = fas + ) +} + +predicate sizeIsOnlyFixedHeader(Expr sizeArg, FlexibleArrayStruct fas) { + exists(SizeofTypeOperator s | + s = sizeArg and typeIsFlexibleStruct(s.getTypeOperand(), fas) + ) + or + exists(SizeofExprOperator s | + s = sizeArg and typeIsFlexibleStruct(s.getExprOperand().getType(), fas) + ) + or + sizeIsOnlyFixedHeader(sizeArg.(Conversion).getExpr(), fas) + or + sizeIsOnlyFixedHeader(sizeArg.(ParenthesisExpr).getExpr(), fas) +} + +predicate bufferIsFlexibleStruct(Expr buf, FlexibleArrayStruct fas) { + typeIsFlexibleStruct(buf.getType(), fas) + or + typeIsFlexibleStruct(buf.(Cast).getType(), fas) + or + bufferIsFlexibleStruct(buf.(Conversion).getExpr(), fas) + or + bufferIsFlexibleStruct(buf.(ParenthesisExpr).getExpr(), fas) +} + +/** + * The same function also byteswaps the FAM field (correct split header/FAM + * pattern). Suppress those. + */ +predicate famIsByteswappedSeparately(ByteswapCall headerSwap, FlexibleArrayStruct fas) { + exists(ByteswapCall famSwap, FieldAccess fa | + famSwap != headerSwap and + famSwap.getEnclosingFunction() = headerSwap.getEnclosingFunction() and + fa.getTarget() = fas.getFam() and + fa.getEnclosingElement*() = famSwap.getBufferArg() + ) +} + +from ByteswapCall c, FlexibleArrayStruct fas, string tname +where + fas.hasStructuredFam() and + tname = min(string n | n = fas.getDisplayName() | n) and + sizeIsOnlyFixedHeader(c.getSizeArg(), fas) and + bufferIsFlexibleStruct(c.getBufferArg(), fas) and + not famIsByteswappedSeparately(c, fas) +select c, + "byteswap of sizeof(" + tname + + ") does not convert flexible array member '" + fas.getFam().getName() + + "' (element type " + fas.getFamElementType().getName() + ") in " + + c.getEnclosingFunction().getName() + "." diff --git a/module/zfs/zfs_replay.c b/module/zfs/zfs_replay.c index ab68d2afc9fb..bcab7102818d 100644 --- a/module/zfs/zfs_replay.c +++ b/module/zfs/zfs_replay.c @@ -1173,11 +1173,16 @@ zfs_replay_clone_range(void *arg1, void *arg2, boolean_t byteswap) int error; ASSERT3U(lr->lr_common.lrc_reclen, >=, sizeof (*lr)); + + if (byteswap) + byteswap_uint64_array(lr, sizeof (*lr)); + ASSERT3U(lr->lr_common.lrc_reclen, >=, offsetof(lr_clone_range_t, lr_bps[lr->lr_nbps])); if (byteswap) - byteswap_uint64_array(lr, sizeof (*lr)); + byteswap_uint64_array(lr->lr_bps, + sizeof (blkptr_t) * lr->lr_nbps); if ((error = zfs_zget(zfsvfs, lr->lr_foid, &zp)) != 0) { /* diff --git a/module/zfs/zil.c b/module/zfs/zil.c index 433d27dd2d10..ab756f117873 100644 --- a/module/zfs/zil.c +++ b/module/zfs/zil.c @@ -672,10 +672,6 @@ zil_claim_clone_range(zilog_t *zilog, const lr_t *lrc, void *tx, return (0); } - /* - * XXX: Do we need to byteswap lr? - */ - for (ii = 0; ii < lr->lr_nbps; ii++) { bp = &lr->lr_bps[ii]; diff --git a/module/zfs/zvol.c b/module/zfs/zvol.c index cb7f2cb246a3..d43d743f7f7d 100644 --- a/module/zfs/zvol.c +++ b/module/zfs/zvol.c @@ -574,11 +574,16 @@ zvol_replay_clone_range(void *arg1, void *arg2, boolean_t byteswap) uint64_t len; ASSERT3U(lr->lr_common.lrc_reclen, >=, sizeof (*lr)); + + if (byteswap) + byteswap_uint64_array(lr, sizeof (*lr)); + ASSERT3U(lr->lr_common.lrc_reclen, >=, offsetof(lr_clone_range_t, lr_bps[lr->lr_nbps])); if (byteswap) - byteswap_uint64_array(lr, sizeof (*lr)); + byteswap_uint64_array(lr->lr_bps, + sizeof (blkptr_t) * lr->lr_nbps); ASSERT(spa_feature_is_enabled(dmu_objset_spa(os), SPA_FEATURE_BLOCK_CLONING));