linux-kernel - Re: [PATCH V2 04/13] selftests/resctrl: Protect against array overrun during iMC config parsing

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <cbfc027c-264d-47f4-949a-3216cd79bf0b@intel.com>
Date: Mon, 30 Sep 2024 09:04:06 -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 V2 04/13] selftests/resctrl: Protect against array overrun
 during iMC config parsing

Hi Ilpo,

On 9/30/24 6:35 AM, Ilpo Järvinen wrote:
> On Thu, 12 Sep 2024, Reinette Chatre wrote:
> 
>> The MBM and MBA tests need to discover the event and umask with which to
>> configure the performance event used to measure read memory bandwidth.
>> This is done by parsing the
>> /sys/bus/event_source/devices/uncore_imc_<imc instance>/events/cas_count_read
>> file for each iMC instance that contains the formatted
>> output: "event=<event>,umask=<umask>"
>>
>> Parsing of cas_count_read contents is done by initializing an array of
>> MAX_TOKENS elements with tokens (deliminated by "=,") from this file.
>> Start by removing the unnecessary append of a delimiter to the string
> 
> Start what? (It sounds odd given the lack of any context, my guess is 
> you're trying to refer to start/first one of the changes you make in the 
> patch but the textual context does not support that conclusion.) I suggest 
> you just rephrase it and avoid using "start" word altogether.

Indeed, I'll just drop the "Start by" and have the sentence be:
"Remove the unnecessary append of a delimiter ..."

> 
>> needing to be parsed. Per the strtok() man page: "delimiter bytes at
>> the start or end of the string are ignored". This has no impact on
>> the token placement within the array.
>>
>> After initialization, the actual event and umask is determined by
>> parsing the tokens directly following the "event" and "umask" tokens
>> respectively.
>>
>> Iterating through the array up to index "i < MAX_TOKENS" but then
>> accessing index "i + 1" risks array overrun during the final iteration.
>> Avoid array overrun by ensuring that the index used within for
>> loop will always be valid.
>>
>> Fixes: 1d3f08687d76 ("selftests/resctrl: Read memory bandwidth from perf IMC counter and from resctrl file system")
>> Signed-off-by: Reinette Chatre <reinette.chatre@...el.com>
>> ---
>> Changes since V1:
>> - New patch.
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index 70e8e31f5d1a..e88d5ca30517 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -83,13 +83,12 @@ static void get_event_and_umask(char *cas_count_cfg, int count, bool op)
>>  	char *token[MAX_TOKENS];
>>  	int i = 0;
>>  
>> -	strcat(cas_count_cfg, ",");
>>  	token[0] = strtok(cas_count_cfg, "=,");
>>  
>>  	for (i = 1; i < MAX_TOKENS; i++)
>>  		token[i] = strtok(NULL, "=,");
>>  
>> -	for (i = 0; i < MAX_TOKENS; i++) {
>> +	for (i = 0; i < MAX_TOKENS - 1; i++) {
>>  		if (!token[i])
>>  			break;
>>  		if (strcmp(token[i], "event") == 0) {
>>
> 
> The code change seems fine so after improving the commit message, please 
> add:
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> 

Thank you very much.

Reinette