We were wasting a byte due to an off-by-one bug.  s[c]nprintf()
doesn't write more than $2 bytes including the null byte, so trying to
pass 'size-1' there is wasting one byte.  Now that we use seprintf(),
the situation isn't different: seprintf() will stop writing *before*
'end' --that is, at most the terminating null byte will be written at
'end-1'--.

Fixes: bc8fbc5f305a (2021-02-26; "kfence: add test suite")
Fixes: 8ed691b02ade (2022-10-03; "kmsan: add tests for KMSAN")
Cc: Kees Cook <kees@kernel.org>
Cc: Christopher Bazley <chris.bazley.wg14@gmail.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Alexander Potapenko <glider@google.com>
Cc: Jann Horn <jannh@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 mm/kfence/kfence_test.c | 4 ++--
 mm/kmsan/kmsan_test.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
index ff734c514c03..f02c3e23638a 100644
--- a/mm/kfence/kfence_test.c
+++ b/mm/kfence/kfence_test.c
@@ -110,7 +110,7 @@ static bool report_matches(const struct expect_report *r)
 
 	/* Title */
 	cur = expect[0];
-	end = &expect[0][sizeof(expect[0]) - 1];
+	end = ENDOF(expect[0]);
 	switch (r->type) {
 	case KFENCE_ERROR_OOB:
 		cur = seprintf(cur, end, "BUG: KFENCE: out-of-bounds %s",
@@ -140,7 +140,7 @@ static bool report_matches(const struct expect_report *r)
 
 	/* Access information */
 	cur = expect[1];
-	end = &expect[1][sizeof(expect[1]) - 1];
+	end = ENDOF(expect[1]);
 
 	switch (r->type) {
 	case KFENCE_ERROR_OOB:
diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
index a062a46b2d24..882500807db8 100644
--- a/mm/kmsan/kmsan_test.c
+++ b/mm/kmsan/kmsan_test.c
@@ -105,7 +105,7 @@ static bool report_matches(const struct expect_report *r)
 
 	/* Title */
 	cur = expected_header;
-	end = &expected_header[sizeof(expected_header) - 1];
+	end = ENDOF(expected_header);
 
 	cur = seprintf(cur, end, "BUG: KMSAN: %s", r->error_type);
 
-- 
2.50.0

On Mon, 7 Jul 2025 at 07:06, Alejandro Colomar <alx@kernel.org> wrote:
>
> We were wasting a byte due to an off-by-one bug.  s[c]nprintf()
> doesn't write more than $2 bytes including the null byte, so trying to
> pass 'size-1' there is wasting one byte.  Now that we use seprintf(),
> the situation isn't different: seprintf() will stop writing *before*
> 'end' --that is, at most the terminating null byte will be written at
> 'end-1'--.
>
> Fixes: bc8fbc5f305a (2021-02-26; "kfence: add test suite")
> Fixes: 8ed691b02ade (2022-10-03; "kmsan: add tests for KMSAN")

Not sure about the Fixes - this means it's likely going to be
backported to stable kernels, which is not appropriate. There's no
functional problem, and these are tests only, so not worth the churn.

Did you run the tests?

Otherwise:

Acked-by: Marco Elver <elver@google.com>

> Cc: Kees Cook <kees@kernel.org>
> Cc: Christopher Bazley <chris.bazley.wg14@gmail.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
>  mm/kfence/kfence_test.c | 4 ++--
>  mm/kmsan/kmsan_test.c   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/mm/kfence/kfence_test.c b/mm/kfence/kfence_test.c
> index ff734c514c03..f02c3e23638a 100644
> --- a/mm/kfence/kfence_test.c
> +++ b/mm/kfence/kfence_test.c
> @@ -110,7 +110,7 @@ static bool report_matches(const struct expect_report *r)
>
>         /* Title */
>         cur = expect[0];
> -       end = &expect[0][sizeof(expect[0]) - 1];
> +       end = ENDOF(expect[0]);
>         switch (r->type) {
>         case KFENCE_ERROR_OOB:
>                 cur = seprintf(cur, end, "BUG: KFENCE: out-of-bounds %s",
> @@ -140,7 +140,7 @@ static bool report_matches(const struct expect_report *r)
>
>         /* Access information */
>         cur = expect[1];
> -       end = &expect[1][sizeof(expect[1]) - 1];
> +       end = ENDOF(expect[1]);
>
>         switch (r->type) {
>         case KFENCE_ERROR_OOB:
> diff --git a/mm/kmsan/kmsan_test.c b/mm/kmsan/kmsan_test.c
> index a062a46b2d24..882500807db8 100644
> --- a/mm/kmsan/kmsan_test.c
> +++ b/mm/kmsan/kmsan_test.c
> @@ -105,7 +105,7 @@ static bool report_matches(const struct expect_report *r)
>
>         /* Title */
>         cur = expected_header;
> -       end = &expected_header[sizeof(expected_header) - 1];
> +       end = ENDOF(expected_header);
>
>         cur = seprintf(cur, end, "BUG: KMSAN: %s", r->error_type);
>
> --
> 2.50.0
>

On Mon 07-07-25 09:46:12, Marco Elver wrote:
> On Mon, 7 Jul 2025 at 07:06, Alejandro Colomar <alx@kernel.org> wrote:
> >
> > We were wasting a byte due to an off-by-one bug.  s[c]nprintf()
> > doesn't write more than $2 bytes including the null byte, so trying to
> > pass 'size-1' there is wasting one byte.  Now that we use seprintf(),
> > the situation isn't different: seprintf() will stop writing *before*
> > 'end' --that is, at most the terminating null byte will be written at
> > 'end-1'--.
> >
> > Fixes: bc8fbc5f305a (2021-02-26; "kfence: add test suite")
> > Fixes: 8ed691b02ade (2022-10-03; "kmsan: add tests for KMSAN")
> 
> Not sure about the Fixes - this means it's likely going to be
> backported to stable kernels, which is not appropriate. There's no
> functional problem, and these are tests only, so not worth the churn.

As long as there is no actual bug fixed then I believe those Fixes tags
are more confusing than actually helpful. And that applies to other
patches in this series as well.
-- 
Michal Hocko
SUSE Labs

Hi Michal,

On Mon, Jul 07, 2025 at 09:53:31AM +0200, Michal Hocko wrote:
> On Mon 07-07-25 09:46:12, Marco Elver wrote:
> > On Mon, 7 Jul 2025 at 07:06, Alejandro Colomar <alx@kernel.org> wrote:
> > >
> > > We were wasting a byte due to an off-by-one bug.  s[c]nprintf()
> > > doesn't write more than $2 bytes including the null byte, so trying to
> > > pass 'size-1' there is wasting one byte.  Now that we use seprintf(),
> > > the situation isn't different: seprintf() will stop writing *before*
> > > 'end' --that is, at most the terminating null byte will be written at
> > > 'end-1'--.
> > >
> > > Fixes: bc8fbc5f305a (2021-02-26; "kfence: add test suite")
> > > Fixes: 8ed691b02ade (2022-10-03; "kmsan: add tests for KMSAN")
> > 
> > Not sure about the Fixes - this means it's likely going to be
> > backported to stable kernels, which is not appropriate. There's no
> > functional problem, and these are tests only, so not worth the churn.
> 
> As long as there is no actual bug fixed then I believe those Fixes tags
> are more confusing than actually helpful. And that applies to other
> patches in this series as well.

For the dead code, I can remove the fixes tags, and even the changes
themselves, since there are good reasons to keep the dead code
(consistency, and avoiding a future programmer forgetting to add it back
when adding a subsequent seprintf() call).

For the fixes to UB, do you prefer the Fixes tags to be removed too?


Have a lovely day!
Alex

> -- 
> Michal Hocko
> SUSE Labs

-- 
<https://www.alejandro-colomar.es/>

On Mon 07-07-25 16:42:43, Alejandro Colomar wrote:
> Hi Michal,
> 
> On Mon, Jul 07, 2025 at 09:53:31AM +0200, Michal Hocko wrote:
> > On Mon 07-07-25 09:46:12, Marco Elver wrote:
> > > On Mon, 7 Jul 2025 at 07:06, Alejandro Colomar <alx@kernel.org> wrote:
> > > >
> > > > We were wasting a byte due to an off-by-one bug.  s[c]nprintf()
> > > > doesn't write more than $2 bytes including the null byte, so trying to
> > > > pass 'size-1' there is wasting one byte.  Now that we use seprintf(),
> > > > the situation isn't different: seprintf() will stop writing *before*
> > > > 'end' --that is, at most the terminating null byte will be written at
> > > > 'end-1'--.
> > > >
> > > > Fixes: bc8fbc5f305a (2021-02-26; "kfence: add test suite")
> > > > Fixes: 8ed691b02ade (2022-10-03; "kmsan: add tests for KMSAN")
> > > 
> > > Not sure about the Fixes - this means it's likely going to be
> > > backported to stable kernels, which is not appropriate. There's no
> > > functional problem, and these are tests only, so not worth the churn.
> > 
> > As long as there is no actual bug fixed then I believe those Fixes tags
> > are more confusing than actually helpful. And that applies to other
> > patches in this series as well.
> 
> For the dead code, I can remove the fixes tags, and even the changes
> themselves, since there are good reasons to keep the dead code
> (consistency, and avoiding a future programmer forgetting to add it back
> when adding a subsequent seprintf() call).
> 
> For the fixes to UB, do you prefer the Fixes tags to be removed too?

Are any of those UB a real or just theoretical problems? To be more
precise I do not question to have those plugged but is there any
evidence that older kernels would need those as well other than just in
case?

-- 
Michal Hocko
SUSE Labs

Hi Michal,

On Mon, Jul 07, 2025 at 05:12:00PM +0200, Michal Hocko wrote:
> > For the dead code, I can remove the fixes tags, and even the changes
> > themselves, since there are good reasons to keep the dead code
> > (consistency, and avoiding a future programmer forgetting to add it back
> > when adding a subsequent seprintf() call).
> > 
> > For the fixes to UB, do you prefer the Fixes tags to be removed too?
> 
> Are any of those UB a real or just theoretical problems? To be more
> precise I do not question to have those plugged but is there any
> evidence that older kernels would need those as well other than just in
> case?

No, I haven't done any checks to verify that this is exploitable in any
way.  I personally wouldn't backport any of this.

About the Fixes: tags, I guess if they are interpreted as something to
be backported, I'll remove them all, as I don't want to backport this.

I guess having them listed in the mailing list archives would be good
enough for speleology purposes (e.g., for someone interested in what
kinds of issues this API fixes).

I'll remove them all.


Cheers,
Alex

> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
<https://www.alejandro-colomar.es/>