CVE-2022-1031: Use After Free (CWE-416)
value: Use After Free in op_is_set_bp in GitHub repository radareorg/radare2 prior to 5.6.6.
CWE description: The product reuses or references memory after it has been freed. At some point afterward, the memory may be allocated again and saved in another pointer, while the original pointer references a location somewhere within the new allocation. Any operations using the original pointer are no longer valid because the memory "belongs" to the code that operates on the new pointer.
CVSS v3 base score: 7.8
Published: 2022-03-22T20:15Z
Repository: https://github.com/radareorg/radare2
Language: C
Files changed: libr/anal/fcn.c, libr/anal/var.c
Commit date: 2022-03-21 18:46:53 +0100
Commit message: Fix UAF in aaaa on arm/thumb switching ##crash

* Reported by @peacock-doris via huntr.dev
* Reproducer tests_65185
* This is a logic fix, but not the fully safe as changes in the code
  can result on UAF again, to properly protect r2 from crashing we
  need to break the ABI and add refcounting to RRegItem, which can't
  happen in 5.6.x because of abi-compat rules

Diff: diff --git a/libr/anal/fcn.c b/libr/anal/fcn.c
index 9780d010464..d159e1ae36c 100644
--- a/libr/anal/fcn.c
+++ b/libr/anal/fcn.c
@@ -813,11 +813,11 @@ repeat:
 			// Come back and handle the current instruction later.
 			// Save the location of it in `delay.idx`
 			// note, we have still increased size of basic block
 			// (and function)
 			if (anal->verbose) {
-				eprintf("Enter branch delay at 0x%08"PFMT64x ". bb->sz=%"PFMT64u"\n", at - oplen, bb->size);
+				eprintf ("Enter branch delay at 0x%08"PFMT64x ". bb->sz=%"PFMT64u"\n", at - oplen, bb->size);
 			}
 			delay.idx = idx - oplen;
 			delay.cnt = op->delay;
 			delay.pending = 1; // we need this in case the actual idx is zero...
 			delay.adjust = !overlapped; // adjustment is required later to avoid double count
@@ -880,10 +880,16 @@ repeat:
 		}
 		if (op->ptr && op->ptr != UT64_MAX && op->ptr != UT32_MAX) {
 			// swapped parameters wtf
 			r_anal_xrefs_set (anal, op->addr, op->ptr, R_ANAL_REF_TYPE_DATA);
 		}
+		if (anal->opt.vars && !varset) {
+			// XXX uses op.src/dst and fails because regprofile invalidates the regitems
+			// lets just call this BEFORE retpoline() to avoid such issue
+			r_anal_extract_vars (anal, fcn, op);
+		}
+		// this call may cause regprofile changes which cause ranalop.regitem references to be invalid
 		analyze_retpoline (anal, op);
 		switch (op->type & R_ANAL_OP_TYPE_MASK) {
 		case R_ANAL_OP_TYPE_CMOV:
 		case R_ANAL_OP_TYPE_MOV:
 			last_is_reg_mov_lea = false;
@@ -971,11 +977,11 @@ repeat:
 			}
 			if (has_stack_regs && op_is_set_bp (op_dst, op_src, bp_reg, sp_reg)     ) {
 				fcn->bp_off = fcn->stack - op->src[0]->delta;
 			}
 			if (op->dst && op->dst->reg && op->dst->reg->name && op->ptr > 0 && op->ptr != UT64_MAX) {
-				free(last_reg_mov_lea_name);
+				free (last_reg_mov_lea_name);
 				if ((last_reg_mov_lea_name = strdup(op->dst->reg->name))) {
 					last_reg_mov_lea_val = op->ptr;
 					last_is_reg_mov_lea = true;
 				}
 			}
@@ -1402,14 +1408,17 @@ analopfinish:
 					fcn->bp_off = fcn->stack + op->src[1]->imm;
 					break;
 				}
 			}
 		}
+#if 0
 		if (anal->opt.vars && !varset) {
 			// XXX uses op.src/dst and fails because regprofile invalidates the regitems
+			// we must ranalop in here to avoid uaf
 			r_anal_extract_vars (anal, fcn, op);
 		}
+#endif
 		if (op->type != R_ANAL_OP_TYPE_MOV && op->type != R_ANAL_OP_TYPE_CMOV && op->type != R_ANAL_OP_TYPE_LEA) {
 			last_is_reg_mov_lea = false;
 		}
 		if (op->type != R_ANAL_OP_TYPE_PUSH && op->type != R_ANAL_OP_TYPE_RPUSH) {
 			last_is_push = false;
diff --git a/libr/anal/var.c b/libr/anal/var.c
index 87f6a601c3f..575e7bb4715 100644
--- a/libr/anal/var.c
+++ b/libr/anal/var.c
@@ -1046,11 +1046,11 @@ static void extract_arg(RAnal *anal, RAnalFunction *fcn, RAnalOp *op, const char
 			free (varname);
 		}
 		free (vartype);
 	} else {
 		st64 frame_off = -(ptr + fcn->bp_off);
-		if (maxstackframe != 0 && (frame_off > maxstackframe || frame_off < -maxstackframe)) {
+		if (maxstackframe > 0 && (frame_off > maxstackframe || frame_off < -maxstackframe)) {
 			goto beach;
 		}
 		RAnalVar *var = get_stack_var (fcn, frame_off);
 		if (var) {
 			r_anal_var_set_access (var, reg, op->addr, rw, -ptr);

Vulnerable code: 				eprintf("Enter branch delay at 0x%08"PFMT64x ". bb->sz=%"PFMT64u"\n", at - oplen, bb->size);
				free(last_reg_mov_lea_name);
		if (maxstackframe != 0 && (frame_off > maxstackframe || frame_off < -maxstackframe)) {
Fixed code: 				eprintf ("Enter branch delay at 0x%08"PFMT64x ". bb->sz=%"PFMT64u"\n", at - oplen, bb->size);
		if (anal->opt.vars && !varset) {
			// XXX uses op.src/dst and fails because regprofile invalidates the regitems
			// lets just call this BEFORE retpoline() to avoid such issue
			r_anal_extract_vars (anal, fcn, op);
		}
		// this call may cause regprofile changes which cause ranalop.regitem references to be invalid
				free (last_reg_mov_lea_name);
#if 0
			// we must ranalop in here to avoid uaf
#endif
		if (maxstackframe > 0 && (frame_off > maxstackframe || frame_off < -maxstackframe)) {