| Message ID | 20230831110843.26719-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Rework benchmark command handling | expand |
Hi Ilpo, On 8/31/2023 4:08 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. Argument size is also fixed by BENCHMARK_ARG_SIZE (63 > bytes of space after fitting the terminating \0 character) and user > could have inputted argument longer than that. > > Return error in case the benchmark command does not fit to the space > allocated for it. > > Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > --- > tools/testing/selftests/resctrl/resctrl_tests.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > index d511daeb6851..a9331b31c32d 100644 > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > @@ -255,9 +255,14 @@ 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) > + ksft_exit_fail_msg("Too long benchmark command.\n"); > + > /* Extract benchmark command from command line. */ > for (i = ben_ind; i < argc; i++) { > benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; > + if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1) Should this perhaps be: if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE) As you note in the longest string that can be fitted should be 63 to account for the \0. If I understand correctly comparing with "BENCHMARK_ARG_SIZE - 1" would would consider a 63 byte string as invalid. > + ksft_exit_fail_msg("Too long benchmark command argument.\n"); > sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]); > } > benchmark_cmd[ben_count] = NULL; Reinette
On Thu, 31 Aug 2023, Reinette Chatre wrote: > Hi Ilpo, > > On 8/31/2023 4:08 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. Argument size is also fixed by BENCHMARK_ARG_SIZE (63 > > bytes of space after fitting the terminating \0 character) and user > > could have inputted argument longer than that. > > > > Return error in case the benchmark command does not fit to the space > > allocated for it. > > > > Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > > Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> > > --- > > tools/testing/selftests/resctrl/resctrl_tests.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c > > index d511daeb6851..a9331b31c32d 100644 > > --- a/tools/testing/selftests/resctrl/resctrl_tests.c > > +++ b/tools/testing/selftests/resctrl/resctrl_tests.c > > @@ -255,9 +255,14 @@ 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) > > + ksft_exit_fail_msg("Too long benchmark command.\n"); > > + > > /* Extract benchmark command from command line. */ > > for (i = ben_ind; i < argc; i++) { > > benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; > > + if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1) > > Should this perhaps be: > if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE) > > As you note in the longest string that can be fitted should be 63 to account for > the \0. If I understand correctly comparing with "BENCHMARK_ARG_SIZE - 1" would > would consider a 63 byte string as invalid. Of course, I don't know why I added that - 1 there.
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index d511daeb6851..a9331b31c32d 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -255,9 +255,14 @@ 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) + ksft_exit_fail_msg("Too long benchmark command.\n"); + /* Extract benchmark command from command line. */ for (i = ben_ind; i < argc; i++) { benchmark_cmd[i - ben_ind] = benchmark_cmd_area[i]; + if (strlen(argv[i]) >= BENCHMARK_ARG_SIZE - 1) + ksft_exit_fail_msg("Too long benchmark command argument.\n"); sprintf(benchmark_cmd[i - ben_ind], "%s", argv[i]); } benchmark_cmd[ben_count] = NULL;
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. Argument size is also fixed by BENCHMARK_ARG_SIZE (63 bytes of space after fitting the terminating \0 character) and user could have inputted argument longer than that. Return error in case the benchmark command does not fit to the space allocated for it. Fixes: ecdbb911f22d ("selftests/resctrl: Add MBM test") Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com> --- tools/testing/selftests/resctrl/resctrl_tests.c | 5 +++++ 1 file changed, 5 insertions(+)