[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <93b9f530-1d2d-dc19-1d48-c15aced32804@linux.intel.com>
Date: Mon, 30 Sep 2024 16:35:28 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Reinette Chatre <reinette.chatre@...el.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
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.
> 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>
--
i.
Powered by blists - more mailing lists