shithub: rgbds

Info  •  Files  •  Log  •  Branches

Fix buffer overflow when creating patches with long RPN expressions

The createpatch() function was using a fixed-size buffer. I've changed it
to be dynamically allocated. I saw that the RPN format used in patches is
slightly different from the one used internally in the assembler, so I
added a new member to the Expression struct to track the patch size.

I've also limited the RPN expression length to 1MB. I realized that the
patch RPN expression could potentially be longer than the internal RPN
expression, so the internal expression would need a limit smaller than
UINT32_MAX. I thought 1MB would be a reasonable limit.

--- a/include/asm/rpn.h

+++ b/include/asm/rpn.h

@@ -11,11 +11,14 @@

 #include <stdint.h>

+#define MAXRPNLEN 1048576

+

 struct Expression {

 	int32_t  nVal;

 	uint8_t  *tRPN;

 	uint32_t nRPNCapacity;

 	uint32_t nRPNLength;

+	uint32_t nRPNPatchSize;

 	uint32_t nRPNOut;

 	uint32_t isReloc;

 };

--- a/src/asm/output.c

+++ b/src/asm/output.c

@@ -10,6 +10,7 @@

  * Outputs an objectfile

  */

+#include <assert.h>

 #include <errno.h>

 #include <stdio.h>

 #include <stdint.h>

@@ -392,10 +393,15 @@

 {

 	struct Patch *pPatch;

 	uint16_t rpndata;

-	uint8_t rpnexpr[2048];

+	uint8_t *rpnexpr;

 	char tzSym[512];

 	uint32_t rpnptr = 0, symptr;

+	rpnexpr = malloc(expr->nRPNPatchSize);

+

+	if (rpnexpr == NULL)

+		fatalerror("No memory for patch RPN expression");

+

 	pPatch = allocpatch();

 	pPatch->nType = type;

 	strcpy(pPatch->tzFilename, tzCurrentFileName);

@@ -477,11 +483,10 @@

 		}

 	}

-	pPatch->pRPN = malloc(rpnptr);

-	if (pPatch->pRPN != NULL) {

-		memcpy(pPatch->pRPN, rpnexpr, rpnptr);

-		pPatch->nRPNSize = rpnptr;

-	}

+	assert(rpnptr == expr->nRPNPatchSize);

+

+	pPatch->pRPN = rpnexpr;

+	pPatch->nRPNSize = rpnptr;

 }

 /*

--- a/src/asm/rpn.c

+++ b/src/asm/rpn.c

@@ -27,7 +27,7 @@

 {

 	assert(src1->tRPN != NULL && src2->tRPN != NULL);

-	if (src1->nRPNLength > UINT32_MAX - src2->nRPNLength)

+	if (src1->nRPNLength + src2->nRPNLength > MAXRPNLEN)

 		fatalerror("RPN expression is too large");

 	uint32_t len = src1->nRPNLength + src2->nRPNLength;

@@ -43,7 +43,7 @@

 		uint32_t cap = (cap1 > cap2) ? cap1 : cap2;

 		if (len > cap)

-			cap = (cap <= UINT32_MAX / 2) ? cap * 2 : len;

+			cap = (cap <= MAXRPNLEN / 2) ? cap * 2 : MAXRPNLEN;

 		expr->nRPNCapacity = cap;

 		expr->tRPN = realloc(expr->tRPN, expr->nRPNCapacity);

@@ -55,6 +55,7 @@

 	free(src2->tRPN);

 	expr->nRPNLength = len;

+	expr->nRPNPatchSize = src1->nRPNPatchSize + src2->nRPNPatchSize;

 	expr->nRPNOut = 0;

 	expr->isReloc = src1->isReloc || src2->isReloc;

 }

@@ -69,8 +70,10 @@

 	if (expr->nRPNLength == expr->nRPNCapacity) {

 		if (expr->nRPNCapacity == 0)

 			expr->nRPNCapacity = 256;

-		else if (expr->nRPNCapacity > UINT32_MAX / 2)

+		else if (expr->nRPNCapacity == MAXRPNLEN)

 			fatalerror("RPN expression is too large");

+		else if (expr->nRPNCapacity > MAXRPNLEN / 2)

+			expr->nRPNCapacity = MAXRPNLEN;

 		else

 			expr->nRPNCapacity *= 2;

 		expr->tRPN = realloc(expr->tRPN, expr->nRPNCapacity);

@@ -90,6 +93,7 @@

 	expr->tRPN = NULL;

 	expr->nRPNCapacity = 0;

 	expr->nRPNLength = 0;

+	expr->nRPNPatchSize = 0;

 	expr->nRPNOut = 0;

 	expr->isReloc = 0;

 }

@@ -134,6 +138,7 @@

 	pushbyte(expr, i >> 16);

 	pushbyte(expr, i >> 24);

 	expr->nVal = i;

+	expr->nRPNPatchSize += 5;

 }

 void rpn_Symbol(struct Expression *expr, char *tzSym)

@@ -146,6 +151,7 @@

 		while (*tzSym)

 			pushbyte(expr, *tzSym++);

 		pushbyte(expr, 0);

+		expr->nRPNPatchSize += 5;

 	} else {

 		rpn_Number(expr, sym_GetConstantValue(tzSym));

 	}

@@ -162,6 +168,7 @@

 	expr->isReloc = 1;

 	pushbyte(expr, RPN_BANK_SELF);

+	expr->nRPNPatchSize++;

 }

 void rpn_BankSymbol(struct Expression *expr, char *tzSym)

@@ -180,6 +187,7 @@

 		while (*tzSym)

 			pushbyte(expr, *tzSym++);

 		pushbyte(expr, 0);

+		expr->nRPNPatchSize += 5;

 	} else {

 		yyerror("BANK argument must be a relocatable identifier");

 	}

@@ -196,9 +204,15 @@

 	expr->isReloc = 1;

 	pushbyte(expr, RPN_BANK_SECT);

-	while (*tzSectionName)

+	expr->nRPNPatchSize++;

+

+	while (*tzSectionName) {

 		pushbyte(expr, *tzSectionName++);

+		expr->nRPNPatchSize++;

+	}

+

 	pushbyte(expr, 0);

+	expr->nRPNPatchSize++;

 }

 void rpn_CheckHRAM(struct Expression *expr, const struct Expression *src)

@@ -205,6 +219,7 @@

 {

 	*expr = *src;

 	pushbyte(expr, RPN_HRAM);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGNOT(struct Expression *expr, const struct Expression *src)

@@ -211,6 +226,7 @@

 {

 	*expr = *src;

 	pushbyte(expr, RPN_LOGUNNOT);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGOR(struct Expression *expr, const struct Expression *src1,

@@ -219,6 +235,7 @@

 	joinexpr();

 	expr->nVal = (expr->nVal || src2->nVal);

 	pushbyte(expr, RPN_LOGOR);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGAND(struct Expression *expr, const struct Expression *src1,

@@ -227,6 +244,7 @@

 	joinexpr();

 	expr->nVal = (expr->nVal && src2->nVal);

 	pushbyte(expr, RPN_LOGAND);

+	expr->nRPNPatchSize++;

 }

 void rpn_HIGH(struct Expression *expr, const struct Expression *src)

@@ -250,6 +268,8 @@

 	pushbyte(expr, 0);

 	pushbyte(expr, RPN_AND);

+

+	expr->nRPNPatchSize += 12;

 }

 void rpn_LOW(struct Expression *expr, const struct Expression *src)

@@ -265,6 +285,8 @@

 	pushbyte(expr, 0);

 	pushbyte(expr, RPN_AND);

+

+	expr->nRPNPatchSize += 6;

 }

 void rpn_LOGEQU(struct Expression *expr, const struct Expression *src1,

@@ -273,6 +295,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal == src2->nVal);

 	pushbyte(expr, RPN_LOGEQ);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGGT(struct Expression *expr, const struct Expression *src1,

@@ -281,6 +304,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal > src2->nVal);

 	pushbyte(expr, RPN_LOGGT);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGLT(struct Expression *expr, const struct Expression *src1,

@@ -289,6 +313,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal < src2->nVal);

 	pushbyte(expr, RPN_LOGLT);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGGE(struct Expression *expr, const struct Expression *src1,

@@ -297,6 +322,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal >= src2->nVal);

 	pushbyte(expr, RPN_LOGGE);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGLE(struct Expression *expr, const struct Expression *src1,

@@ -305,6 +331,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal <= src2->nVal);

 	pushbyte(expr, RPN_LOGLE);

+	expr->nRPNPatchSize++;

 }

 void rpn_LOGNE(struct Expression *expr, const struct Expression *src1,

@@ -313,6 +340,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal != src2->nVal);

 	pushbyte(expr, RPN_LOGNE);

+	expr->nRPNPatchSize++;

 }

 void rpn_ADD(struct Expression *expr, const struct Expression *src1,

@@ -321,6 +349,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal + src2->nVal);

 	pushbyte(expr, RPN_ADD);

+	expr->nRPNPatchSize++;

 }

 void rpn_SUB(struct Expression *expr, const struct Expression *src1,

@@ -329,6 +358,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal - src2->nVal);

 	pushbyte(expr, RPN_SUB);

+	expr->nRPNPatchSize++;

 }

 void rpn_XOR(struct Expression *expr, const struct Expression *src1,

@@ -337,6 +367,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal ^ src2->nVal);

 	pushbyte(expr, RPN_XOR);

+	expr->nRPNPatchSize++;

 }

 void rpn_OR(struct Expression *expr, const struct Expression *src1,

@@ -345,6 +376,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal | src2->nVal);

 	pushbyte(expr, RPN_OR);

+	expr->nRPNPatchSize++;

 }

 void rpn_AND(struct Expression *expr, const struct Expression *src1,

@@ -353,6 +385,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal & src2->nVal);

 	pushbyte(expr, RPN_AND);

+	expr->nRPNPatchSize++;

 }

 void rpn_SHL(struct Expression *expr, const struct Expression *src1,

@@ -370,6 +403,7 @@

 	expr->nVal = (src1->nVal << src2->nVal);

 	pushbyte(expr, RPN_SHL);

+	expr->nRPNPatchSize++;

 }

 void rpn_SHR(struct Expression *expr, const struct Expression *src1,

@@ -383,6 +417,7 @@

 	expr->nVal = (src1->nVal >> src2->nVal);

 	pushbyte(expr, RPN_SHR);

+	expr->nRPNPatchSize++;

 }

 void rpn_MUL(struct Expression *expr, const struct Expression *src1,

@@ -391,6 +426,7 @@

 	joinexpr();

 	expr->nVal = (src1->nVal * src2->nVal);

 	pushbyte(expr, RPN_MUL);

+	expr->nRPNPatchSize++;

 }

 void rpn_DIV(struct Expression *expr, const struct Expression *src1,

@@ -402,6 +438,7 @@

 	expr->nVal = (src1->nVal / src2->nVal);

 	pushbyte(expr, RPN_DIV);

+	expr->nRPNPatchSize++;

 }

 void rpn_MOD(struct Expression *expr, const struct Expression *src1,

@@ -413,6 +450,7 @@

 	expr->nVal = (src1->nVal % src2->nVal);

 	pushbyte(expr, RPN_MOD);

+	expr->nRPNPatchSize++;

 }

 void rpn_UNNEG(struct Expression *expr, const struct Expression *src)

@@ -420,6 +458,7 @@

 	*expr = *src;

 	expr->nVal = -expr->nVal;

 	pushbyte(expr, RPN_UNSUB);

+	expr->nRPNPatchSize++;

 }

 void rpn_UNNOT(struct Expression *expr, const struct Expression *src)

@@ -427,4 +466,5 @@

 	*expr = *src;

 	expr->nVal = ~expr->nVal;

 	pushbyte(expr, RPN_UNNOT);

+	expr->nRPNPatchSize++;

 }

--- /dev/null

+++ b/test/asm/long-rpn-expression.asm

@@ -1,0 +1,33 @@

+SECTION "sec", ROM0

+

+X0 EQUS "0"

+

+m: MACRO

+\1 EQUS STRCAT("{X\2}", "+0")

+ENDM

+

+n = 0

+

+REPT $7E

+n1 = n + 1

+NSTR EQUS STRSUB("{n}", 2, STRLEN("{n}") - 1)

+N1STR EQUS STRSUB("{n1}", 2, STRLEN("{n1}") - 1)

+XN1 EQUS STRCAT("X", "{N1STR}")

+    m XN1, {NSTR}

+    PURGE NSTR, N1STR, XN1

+n = n + 1

+ENDR

+

+; string of 127 zeros separated by plus signs

+X EQUS "{X7E}"

+

+    db x+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+\

+       X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+\

+       X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+\

+       X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+\

+       X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+\

+       X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+\

+       X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+\

+       X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X+X

+

+x db 0