Hi Ilpo,
On 8/8/2023 2:16 AM, Ilpo Järvinen wrote:
> Benchmark command is copied into an array in the stack. The array is
> BENCHMARK_ARGS items long but the command line could try to provide a
> longer command.
>
> Return error in case the benchmark command does not fit to its array.
>
> Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> ---
> tools/testing/selftests/resctrl/resctrl_tests.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index d511daeb6851..eef9e02516ad 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -255,6 +255,9 @@ int main(int argc, char **argv)
> return ksft_exit_skip("Not running as root. Skipping...\n");
>
> if (has_ben) {
> + if (argc - ben_ind >= BENCHMARK_ARGS - 1)
> + ksft_exit_fail_msg("Too long benchmark command");
> +
I think there are two potential issues here: too many arguments and too
long arguments. Current code can handle 64 (63 with last required to be
NULL) arguments each expected to be 64 bytes (63 to end with \0).
The above fix looks to be handling the first issue, with this the
error message could be more accurate if it refers to the
number of arguments instead. It looks to me as though the latter
issue still needs to be handled. I understand that this becomes
unnecessary via the refactor in following patches but I expect that
this fix needs to be backported (cc. stable also) and thus
it may benefit from a precision added to the sprintf() that follows
the snippet below?
> /* Extract benchmark command from command line. */
> for (i = ben_ind; i < argc; i++) {
> benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i];
Reinette
ps. Unless you have an updated email address that works, could you please
remove Sai's email from future submissions?
