| Message ID | 87470c06-b45a-4e83-92ff-aac2e7b9c6ba@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/mm: compaction_test: Fix off by one in check_compaction() | expand |
On 8/9/24 06:32, Dan Carpenter wrote: > The "initial_nr_hugepages" variable is unsigned long so it takes up to > 20 characters to print, plus 1 more character for the NUL terminator. > Unfortunately, this buffer is not quite large enough for the terminator > to fit. Also use snprintf() for a belt and suspenders approach. > > Fixes: fb9293b6b015 ("selftests/mm: compaction_test: fix bogus test success and reduce probability of OOM-killer invocation") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > tools/testing/selftests/mm/compaction_test.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c > index e140558e6f53..2c3a0eb6b22d 100644 > --- a/tools/testing/selftests/mm/compaction_test.c > +++ b/tools/testing/selftests/mm/compaction_test.c > @@ -89,9 +89,10 @@ int check_compaction(unsigned long mem_free, unsigned long hugepage_size, > int fd, ret = -1; > int compaction_index = 0; > char nr_hugepages[20] = {0}; > - char init_nr_hugepages[20] = {0}; > + char init_nr_hugepages[24] = {0}; Can we exceed this limit too? Can you make this a define? > > - sprintf(init_nr_hugepages, "%lu", initial_nr_hugepages); > + snprintf(init_nr_hugepages, sizeof(init_nr_hugepages), > + "%lu", initial_nr_hugepages); > > /* We want to test with 80% of available memory. Else, OOM killer comes > in to play */ With that change: Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> thanks, -- Shuah
On Fri, Aug 09, 2024 at 11:20:48AM -0600, Shuah Khan wrote: > On 8/9/24 06:32, Dan Carpenter wrote: > > The "initial_nr_hugepages" variable is unsigned long so it takes up to > > 20 characters to print, plus 1 more character for the NUL terminator. > > Unfortunately, this buffer is not quite large enough for the terminator > > to fit. Also use snprintf() for a belt and suspenders approach. > > > > Fixes: fb9293b6b015 ("selftests/mm: compaction_test: fix bogus test success and reduce probability of OOM-killer invocation") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > tools/testing/selftests/mm/compaction_test.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c > > index e140558e6f53..2c3a0eb6b22d 100644 > > --- a/tools/testing/selftests/mm/compaction_test.c > > +++ b/tools/testing/selftests/mm/compaction_test.c > > @@ -89,9 +89,10 @@ int check_compaction(unsigned long mem_free, unsigned long hugepage_size, > > int fd, ret = -1; > > int compaction_index = 0; > > char nr_hugepages[20] = {0}; > > - char init_nr_hugepages[20] = {0}; > > + char init_nr_hugepages[24] = {0}; > > Can we exceed this limit too? Can you make this a define? > It's based on counting the digits in U64_MAX. 18446744073709551615X 123456789012345678901 We don't have any defines for that kind of thing. It's not a bad idea. #define STRLEN_U64_MAX 20 char init_nr_hugepages[STRLEN_U64_MAX + 1]; But it should be done as part of a kernel wide clean up and not part of this buffer overflow fix. The line above it could be changed as well, for example. Let me create a KTODO and hope the internet will take care of it. KTODO: create defines for STRLEN_[SU]8/16/32/64_MIN/MAX. Btw, I rounded up to 24 just because I like buffer sizes that are divisible by 4 but the compiler is probably going to do that automatically either way. regards, dan carpenter
diff --git a/tools/testing/selftests/mm/compaction_test.c b/tools/testing/selftests/mm/compaction_test.c index e140558e6f53..2c3a0eb6b22d 100644 --- a/tools/testing/selftests/mm/compaction_test.c +++ b/tools/testing/selftests/mm/compaction_test.c @@ -89,9 +89,10 @@ int check_compaction(unsigned long mem_free, unsigned long hugepage_size, int fd, ret = -1; int compaction_index = 0; char nr_hugepages[20] = {0}; - char init_nr_hugepages[20] = {0}; + char init_nr_hugepages[24] = {0}; - sprintf(init_nr_hugepages, "%lu", initial_nr_hugepages); + snprintf(init_nr_hugepages, sizeof(init_nr_hugepages), + "%lu", initial_nr_hugepages); /* We want to test with 80% of available memory. Else, OOM killer comes in to play */
The "initial_nr_hugepages" variable is unsigned long so it takes up to 20 characters to print, plus 1 more character for the NUL terminator. Unfortunately, this buffer is not quite large enough for the terminator to fit. Also use snprintf() for a belt and suspenders approach. Fixes: fb9293b6b015 ("selftests/mm: compaction_test: fix bogus test success and reduce probability of OOM-killer invocation") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- tools/testing/selftests/mm/compaction_test.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)