linux-kernel - Re: [PATCH V3 10/15] selftests/resctrl: Make benchmark parameter passing robust

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <104e159a-80fb-4ad5-ae9e-3e5f549a3535@intel.com>
Date: Fri, 18 Oct 2024 10:34:25 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <fenghua.yu@...el.com>, <shuah@...nel.org>, <tony.luck@...el.com>,
	<peternewman@...gle.com>, <babu.moger@....com>,
	Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>,
	<linux-kselftest@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 10/15] selftests/resctrl: Make benchmark parameter
 passing robust

Hi Ilpo,

On 10/18/24 2:03 AM, Ilpo Järvinen wrote:
> On Thu, 17 Oct 2024, Reinette Chatre wrote:

>> +/*
>> + * Allocate and initialize a struct fill_buf_param with user provided
>> + * (via "-b fill_buf <fill_buf parameters>") parameters.
>> + *
>> + * Use defaults (that may not be appropriate for all tests) for any
>> + * fill_buf parameters omitted by the user.
>> + *
>> + * Historically it may have been possible for user space to provide
>> + * additional parameters, "operation" ("read" vs "write") in
>> + * benchmark_cmd[3] and "once" (run "once" or until terminated) in
>> + * benchmark_cmd[4]. Changing these parameters have never been
>> + * supported with the default of "read" operation and running until
>> + * terminated built into the tests. Any unsupported values for
>> + * (original) "fill_buf" parameters are treated as failure.
>> + *
>> + * Return: On failure, forcibly exits the test on any parsing failure,
>> + *         returns NULL if no parsing needed (user did not actually provide
>> + *         "-b fill_buf").
>> + *         On success, returns pointer to newly allocated and fully
>> + *         initialized struct fill_buf_param that caller must free.
>> + */
>> +static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
>> +{
>> +	struct fill_buf_param *fill_param = NULL;
>> +	char *endptr = NULL;
>> +
>> +	if (!uparams->benchmark_cmd[0] || strcmp(uparams->benchmark_cmd[0], "fill_buf"))
>> +		return NULL;
>> +
>> +	fill_param = malloc(sizeof(*fill_param));
>> +	if (!fill_param)
>> +		ksft_exit_skip("Unable to allocate memory for fill_buf parameters.\n");
>> +
>> +	if (uparams->benchmark_cmd[1]) {
>> +		errno = 0;
>> +		fill_param->buf_size = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
>> +		if (errno || *endptr != '\0') {
> 
> Same here as with the patch 2 (and also in the checks below), both empty 
> string and extra character checks are necessary.

Thank you very much. Addressed with fixup below:

@@ -181,7 +181,7 @@ static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
 	if (!fill_param)
 		ksft_exit_skip("Unable to allocate memory for fill_buf parameters.\n");
 
-	if (uparams->benchmark_cmd[1]) {
+	if (uparams->benchmark_cmd[1] && *uparams->benchmark_cmd[1] != '\0') {
 		errno = 0;
 		fill_param->buf_size = strtoul(uparams->benchmark_cmd[1], &endptr, 10);
 		if (errno || *endptr != '\0') {
@@ -192,7 +192,7 @@ static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
 		fill_param->buf_size = MINIMUM_SPAN;
 	}
 
-	if (uparams->benchmark_cmd[2]) {
+	if (uparams->benchmark_cmd[2] && *uparams->benchmark_cmd[2] != '\0') {
 		errno = 0;
 		fill_param->memflush = strtol(uparams->benchmark_cmd[2], &endptr, 10) != 0;
 		if (errno || *endptr != '\0') {
@@ -203,14 +203,14 @@ static struct fill_buf_param *alloc_fill_buf_param(struct user_params *uparams)
 		fill_param->memflush = true;
 	}
 
-	if (uparams->benchmark_cmd[3]) {
+	if (uparams->benchmark_cmd[3] && *uparams->benchmark_cmd[3] != '\0') {
 		if (strcmp(uparams->benchmark_cmd[3], "0")) {
 			free(fill_param);
 			ksft_exit_skip("Only read operations supported.\n");
 		}
 	}
 
-	if (uparams->benchmark_cmd[4]) {
+	if (uparams->benchmark_cmd[4] && *uparams->benchmark_cmd[4] != '\0') {
 		if (strcmp(uparams->benchmark_cmd[4], "false")) {
 			free(fill_param);
 			ksft_exit_skip("fill_buf is required to run until termination.\n");

Reinette