Fix incorrect mem access width guesses for ARM thumb.

rizinorg · Dec 12, 2023 · 96038ea · 96038ea
1 parent 899794e
commit 96038ea
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 8 deletions.
diff --git a/librz/analysis/analysis.c b/librz/analysis/analysis.c
@@ -13,6 +13,23 @@ RZ_LIB_VERSION(rz_analysis);
 
 static RzAnalysisPlugin *analysis_static_plugins[] = { RZ_ANALYSIS_STATIC_PLUGINS };
 
+/**
+ * \brief Returns the default size byte width of memory access operations.
+ * The size is just a best guess.
+ *
+ * \param analysis The current RzAnalysis in use.
+ *
+ * \return The default width of a memory access in bytes.
+ */
+RZ_API ut32 rz_analysis_guessed_mem_access_width(RZ_NONNULL const RzAnalysis *analysis) {
+	if (analysis->bits == 16 && RZ_STR_EQ(analysis->cur->arch, "arm")) {
+		// Thumb access is usually 4 bytes of memory by default.
+		return 4;
+	}
+	// Best guess for variable size.
+	return analysis->bits / 8;
+}
+
 RZ_API void rz_analysis_set_limits(RzAnalysis *analysis, ut64 from, ut64 to) {
 	free(analysis->limit);
 	analysis->limit = RZ_NEW0(RzAnalysisRange);

diff --git a/librz/analysis/op.c b/librz/analysis/op.c
@@ -31,6 +31,7 @@ RZ_API void rz_analysis_op_init(RzAnalysisOp *op) {
 		op->val = UT64_MAX;
 		op->disp = UT64_MAX;
 		op->mmio_address = UT64_MAX;
+		op->stackptr = RZ_ANALYSIS_OP_INVALID_STACKPTR;
 	}
 }
 
@@ -213,6 +214,8 @@ RZ_API bool rz_analysis_op_ismemref(int t) {
 	case RZ_ANALYSIS_OP_TYPE_STORE:
 	case RZ_ANALYSIS_OP_TYPE_LEA:
 	case RZ_ANALYSIS_OP_TYPE_CMP:
+	case RZ_ANALYSIS_OP_TYPE_POP:
+	case RZ_ANALYSIS_OP_TYPE_PUSH:
 		return true;
 	default:
 		return false;

diff --git a/librz/analysis/var.c b/librz/analysis/var.c
@@ -1079,7 +1079,7 @@ static inline bool is_not_read_nor_write(const RzAnalysisOpDirection direction)
 }
 
 /**
- * Try to extract any args from a single op
+ * \brief Try to extract any args from a single op
  *
  * \param reg name of the register to look at for accesses
  * \param from_sp whether \p reg is the sp or bp
@@ -1178,7 +1178,12 @@ static void extract_stack_var(RzAnalysis *analysis, RzAnalysisFunction *fcn, RzA
 
 	RzStackAddr stack_off;
 	if (is_sp) {
-		stack_off = addend - fcn->stack;
+		if (op->stackptr != RZ_ANALYSIS_OP_INVALID_STACKPTR) {
+			// If the stackptr is set we assume it is more correct and use it here.
+			stack_off = op->stackptr;
+		} else {
+			stack_off = addend - fcn->stack;
+		}
 	} else {
 		stack_off = addend - fcn->bp_off;
 	}
@@ -1241,7 +1246,7 @@ static void extract_stack_var(RzAnalysis *analysis, RzAnalysisFunction *fcn, RzA
 		if (varname) {
 			RzAnalysisVarStorage stor;
 			rz_analysis_var_storage_init_stack(&stor, stack_off);
-			RzAnalysisVar *var = rz_analysis_function_set_var(fcn, &stor, vartype, analysis->bits / 8, varname);
+			RzAnalysisVar *var = rz_analysis_function_set_var(fcn, &stor, vartype, rz_analysis_guessed_mem_access_width(analysis), varname);
 			if (var) {
 				rz_analysis_var_set_access(var, reg, op->addr, rw, addend);
 			}
@@ -1257,7 +1262,7 @@ static void extract_stack_var(RzAnalysis *analysis, RzAnalysisFunction *fcn, RzA
 		}
 		char *varname = rz_str_newf("%s_%" PFMT64x "h", VARPREFIX, RZ_ABS(stor.stack_off));
 		if (varname) {
-			RzAnalysisVar *var = rz_analysis_function_set_var(fcn, &stor, NULL, analysis->bits / 8, varname);
+			RzAnalysisVar *var = rz_analysis_function_set_var(fcn, &stor, NULL, rz_analysis_guessed_mem_access_width(analysis), varname);
 			if (var) {
 				rz_analysis_var_set_access(var, reg, op->addr, rw, addend);
 			}

diff --git a/librz/core/canalysis.c b/librz/core/canalysis.c
@@ -3068,7 +3068,7 @@ static int esilbreak_reg_write(RzAnalysisEsil *esil, const char *name, ut64 *val
 	EsilBreakCtx *ctx = esil->user;
 	RzAnalysisOp *op = ctx->op;
 	RzCore *core = analysis->coreb.core;
-	handle_var_stack_access(esil, *val, RZ_ANALYSIS_VAR_ACCESS_TYPE_PTR, esil->analysis->bits / 8);
+	handle_var_stack_access(esil, *val, RZ_ANALYSIS_VAR_ACCESS_TYPE_PTR, rz_analysis_guessed_mem_access_width(esil->analysis));
 	// specific case to handle blx/bx cases in arm through emulation
 	//  XXX this thing creates a lot of false positives
 	ut64 at = *val;

diff --git a/librz/include/rz_analysis.h b/librz/include/rz_analysis.h
@@ -10,6 +10,8 @@
 // still required by core in lot of places
 #define USE_VARSUBS 0
 
+#define RZ_ANALYSIS_OP_INVALID_STACKPTR 0
+
 #include <rz_types.h>
 #include <rz_io.h>
 #include <rz_reg.h>
@@ -719,6 +721,8 @@ typedef enum rz_analysis_var_kind_t {
 	RZ_ANALYSIS_VAR_KIND_END ///< Number of RzAnalysisVarKind enums
 } RzAnalysisVarKind;
 
+RZ_API ut32 rz_analysis_guessed_mem_access_width(RZ_NONNULL const RzAnalysis *analysis);
+
 typedef struct dwarf_variable_t {
 	ut64 offset; ///< DIE offset of the variable
 	RzBinDwarfLocation *location; ///< location description

diff --git a/test/db/analysis/vars b/test/db/analysis/vars
@@ -408,7 +408,6 @@ afvW
 EOF
 RUN
 
-
 NAME=Variable access with misc registers (ARM)
 FILE==
 ARGS=-a arm -b 16
@@ -445,11 +444,11 @@ afvW
 |           ; arg int16_t arg1 @ r0
 |           ; arg int16_t arg2 @ r1
 |           ; arg int16_t arg3 @ r2
-|           ; var int16_t var_10h @ stack - 0x10
+|           ; var int32_t var_10h @ stack - 0x10
 |           ; var int8_t var_fh @ stack - 0xf
 |           ; var int16_t var_eh @ stack - 0xe
 |           ; var int32_t var_ch @ stack - 0xc
-|           ; var int16_t var_4h @ stack - 0x4
+|           ; var int32_t var_4h @ stack - 0x4
 |           0x00000000      80b4           push  {r7}
 |           0x00000002      83b0           sub   sp, 0xc
 |           0x00000004      00af           add   r7, var_10h