linux-kernel - Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits to its array

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]

Date:   Mon, 14 Aug 2023 10:48:40 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        <linux-kselftest@...r.kernel.org>, Shuah Khan <shuah@...nel.org>,
        Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>,
        Babu Moger <babu.moger@....com>, <linux-kernel@...r.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands fits
 to its array

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@...ux.intel.com>
> ---
>  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?