CRASH caused by: [PATCH 06/10] setexpr: Add some tests for buffer overflow and backref

[Heinrich Schuchardt]

On 11/1/20 10:15 PM, Simon Glass wrote:
> Add tests to check for buffer overflow using simple replacement as well
> as back references. At present these don't fully pass.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
>   cmd/setexpr.c      | 21 +++--------
>   include/command.h  | 17 +++++++++
>   test/cmd/setexpr.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 110 insertions(+), 17 deletions(-)
>
> diff --git a/cmd/setexpr.c b/cmd/setexpr.c
> index fe3435b4d99..dbb43b3be2f 100644
> --- a/cmd/setexpr.c
> +++ b/cmd/setexpr.c
> @@ -134,22 +134,8 @@ static char *substitute(char *string, int *slen, int ssize,
>   	return p + nlen;
>   }
>
> -/**
> - * regex_sub() - Replace a regex pattern with a string
> - *
> - * @data: Buffer containing the string to update
> - * @data_size: Size of buffer (must be large enough for the new string)
> - * @nbuf: Back-reference buffer
> - * @nbuf_size: Size of back-reference buffer (must be larger enough for @s plus
> - *	all back-reference expansions)
> - * @r: Regular expression to find
> - * @s: String to replace with
> - * @global: true to replace all matches in @data, false to replace just the
> - *	first
> - * @return 0 if OK, 1 on error
> - */
> -static int regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
> -		     const char *r, const char *s, bool global)
> +int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
> +		      const char *r, const char *s, bool global)
>   {
>   	struct slre slre;
>   	char *datap = data;
> @@ -325,7 +311,8 @@ static int regex_sub_var(const char *name, const char *r, const char *s,
>
>   	strcpy(data, t);
>
> -	ret = regex_sub(data, SLRE_BUFSZ, nbuf, SLRE_PATSZ, r, s, global);
> +	ret = setexpr_regex_sub(data, SLRE_BUFSZ, nbuf, SLRE_PATSZ, r, s,
> +				global);
>   	if (ret)
>   		return 1;
>
> diff --git a/include/command.h b/include/command.h
> index e900f97df33..e229bf2825c 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -183,6 +183,23 @@ extern int do_env_set_efi(struct cmd_tbl *cmdtp, int flag, int argc,
>   			  char *const argv[]);
>   #endif
>
> +/**
> + * setexpr_regex_sub() - Replace a regex pattern with a string
> + *
> + * @data: Buffer containing the string to update
> + * @data_size: Size of buffer (must be large enough for the new string)
> + * @nbuf: Back-reference buffer
> + * @nbuf_size: Size of back-reference buffer (must be larger enough for @s plus
> + *	all back-reference expansions)
> + * @r: Regular expression to find
> + * @s: String to replace with
> + * @global: true to replace all matches in @data, false to replace just the
> + *	first
> + * @return 0 if OK, 1 on error
> + */
> +int setexpr_regex_sub(char *data, uint data_size, char *nbuf, uint nbuf_size,
> +		      const char *r, const char *s, bool global);
> +
>   /*
>    * Error codes that commands return to cmd_process(). We use the standard 0
>    * and 1 for success and failure, but add one more case - failure with a
> diff --git a/test/cmd/setexpr.c b/test/cmd/setexpr.c
> index de54561917c..a6940fd82dd 100644
> --- a/test/cmd/setexpr.c
> +++ b/test/cmd/setexpr.c
> @@ -209,6 +209,95 @@ static int setexpr_test_regex_inc(struct unit_test_state *uts)
>   }
>   SETEXPR_TEST(setexpr_test_regex_inc, UT_TESTF_CONSOLE_REC);
>
> +/* Test setexpr_regex_sub() directly to check buffer usage */
> +static int setexpr_test_sub(struct unit_test_state *uts)
> +{
> +	char *buf, *nbuf;
> +	int i;
> +
> +	buf = map_sysmem(0, BUF_SIZE);
> +	nbuf = map_sysmem(0x1000, BUF_SIZE);
> +
> +	/* Add a pattern so we can check the buffer limits */
> +	memset(buf, '\xff', BUF_SIZE);
> +	memset(nbuf, '\xff', BUF_SIZE);
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		buf[i] = i & 0xff;
> +		nbuf[i] = i & 0xff;
> +	}
> +	strcpy(buf, "this is a test");
> +
> +	/*
> +	 * This is a regression test, since a bug was found in the use of
> +	 * memmove() in setexpr
> +	 */
> +	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE, "is",
> +				      "us it is longer", true));
> +	ut_asserteq_str("thus it is longer us it is longer a test", buf);
> +
> +	/* The following checks fail at present due to a bug in setexpr */
> +	return 0;
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		ut_assertf(buf[i] == (char)i,
> +			   "buf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)buf[i]);
> +		ut_assertf(nbuf[i] == (char)i,
> +			   "nbuf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)nbuf[i]);
> +	}
> +
> +	unmap_sysmem(buf);
> +
> +	return 0;
> +}
> +SETEXPR_TEST(setexpr_test_sub, UT_TESTF_CONSOLE_REC);
> +
> +/* Test setexpr_regex_sub() with back references */
> +static int setexpr_test_backref(struct unit_test_state *uts)
> +{
> +	char *buf, *nbuf;
> +	int i;
> +
> +	buf = map_sysmem(0, BUF_SIZE);
> +	nbuf = map_sysmem(0x1000, BUF_SIZE);

This test is compiled for all boards enabling CONFIG_UNIT_TEST and
CONFIG_CMDLINE.

On the sipeed_maix_smode_defconfig trying to access NULL results in a crash:

Test: setexpr_test_backref
Unhandled exception: Store/AMO access fault
EPC: 00000000805b1152 RA: 00000000805b3bce TVAL: 0000000000001000
EPC: 0000000080056152 RA: 0000000080058bce reloc adjusted

Why did you opt for hard-coded addresses instead of malloc()?

We should be able to run the C unit tests on all boards not only on the
sandbox except where sandbox drivers are involved.

Could you, please, provide a correction.

Best regards

Heinrich

> +
> +	/* Add a pattern so we can check the buffer limits */
> +	memset(buf, '\xff', BUF_SIZE);
> +	memset(nbuf, '\xff', BUF_SIZE);
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		buf[i] = i & 0xff;
> +		nbuf[i] = i & 0xff;
> +	}
> +	strcpy(buf, "this is surely a test is it? yes this is indeed a test");
> +
> +	/*
> +	 * This is a regression test, since a bug was found in the use of
> +	 * memmove() in setexpr
> +	 */
> +	ut_assertok(setexpr_regex_sub(buf, BUF_SIZE, nbuf, BUF_SIZE,
> +				      "(this) (is) (surely|indeed)",
> +				      "us \\1 \\2 \\3!", true));
> +
> +	/* The following checks fail at present due to bugs in setexpr */
> +	return 0;
> +	ut_asserteq_str("us this is surely! a test is it? yes us this is indeed! a test",
> +			buf);
> +
> +	for (i = BUF_SIZE; i < 0x1000; i++) {
> +		ut_assertf(buf[i] == (char)i,
> +			   "buf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)buf[i]);
> +		ut_assertf(nbuf[i] == (char)i,
> +			   "nbuf byte at %x should be %02x, got %02x)\n",
> +			   i, i & 0xff, (u8)nbuf[i]);
> +	}
> +
> +	unmap_sysmem(buf);
> +
> +	return 0;
> +}
> +SETEXPR_TEST(setexpr_test_backref, UT_TESTF_CONSOLE_REC);
> +
>   int do_ut_setexpr(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   {
>   	struct unit_test *tests = ll_entry_start(struct unit_test,
>