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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <12ce6b7a-292c-6f27-809e-a7cbb810f596@linux.intel.com>
Date:   Tue, 15 Aug 2023 12:10:55 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Reinette Chatre <reinette.chatre@...el.com>
cc:     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>,
        LKML <linux-kernel@...r.kernel.org>,
        Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH 1/7] selftests/resctrl: Ensure the benchmark commands
 fits to its array

On Mon, 14 Aug 2023, Reinette Chatre wrote:
> 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?

Thanks for taking a look, yes, both are problems. Third problem which 
still remains is if "fill_buf" is the first argument of -b, the arguments 
are not validated to match the formatting used internally (I guess it 
might be intentional to allow overriding the internal fill_buf arguments 
but just the validation is lacking).

I'll add strlen() check instead of using sprintf() in order to properly 
fail rather than silently truncating the arguments.

> >  		/* 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?

It's auto-added by git send-email machinery. I guess I can try to make 
an exception to my usual workflow by sending only to manually specified To 
addresses (if I remember). Perhaps one day I'll write a tool to filter out
the addresses from git send-email generated ones but as is I don't have 
one.

-- 
 i.