All of lore.kernel.org
 help / color / mirror / Atom feed
* [bug report] printk: ringbuffer: Add KUnit test
@ 2025-06-25 15:22 Dan Carpenter
  2025-06-26  6:59 ` Thomas Weißschuh
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2025-06-25 15:22 UTC (permalink / raw)
  To: thomas.weissschuh; +Cc: John Ogness, Kees Cook, linux-hardening

Hello Thomas Weißschuh,

The patch 5ea2bcdfbf46: "printk: ringbuffer: Add KUnit test" from Jun
12, 2025, leads to the following static checker warning:

	kernel/printk/printk_ringbuffer_kunit_test.c:91 prbtest_check_data()
	(unpublished script worries this an off by one)

kernel/printk/printk_ringbuffer_kunit_test.c
    83 static bool prbtest_check_data(const struct prbtest_rbdata *dat)
    84 {
    85 	unsigned int len;
    86 
    87 	/* Sane length? */
    88 	if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
    89 		return false;
    90 
--> 91 	if (dat->text[dat->len] != '\0')
    92 		return false;
    93 

My question is that the prbtest_rbdata structure is declared like this:

    53  /* test data structure */
    54  struct prbtest_rbdata {
    55          unsigned int len;
    56          char text[] __counted_by(len);
    57  };

The size of text is not really counted by len, it's "MAX_RBDATA_TEXT_SIZE
+ 1".  The condition "if (dat->text[dat->len] != '\0')" is reading one
element beyond the __counted_by() value so something should complain if
we enable all the debugging, right?

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] printk: ringbuffer: Add KUnit test
  2025-06-25 15:22 [bug report] printk: ringbuffer: Add KUnit test Dan Carpenter
@ 2025-06-26  6:59 ` Thomas Weißschuh
  2025-07-01 15:23   ` Dan Carpenter
                     ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Thomas Weißschuh @ 2025-06-26  6:59 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: John Ogness, Kees Cook, linux-hardening, Petr Mladek

On Wed, Jun 25, 2025 at 10:22:19AM -0500, Dan Carpenter wrote:
> Hello Thomas Weißschuh,
> 
> The patch 5ea2bcdfbf46: "printk: ringbuffer: Add KUnit test" from Jun
> 12, 2025, leads to the following static checker warning:
> 
> 	kernel/printk/printk_ringbuffer_kunit_test.c:91 prbtest_check_data()
> 	(unpublished script worries this an off by one)
> 
> kernel/printk/printk_ringbuffer_kunit_test.c
>     83 static bool prbtest_check_data(const struct prbtest_rbdata *dat)
>     84 {
>     85 	unsigned int len;
>     86 
>     87 	/* Sane length? */
>     88 	if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
>     89 		return false;
>     90 
> --> 91 	if (dat->text[dat->len] != '\0')
>     92 		return false;
>     93 
> 
> My question is that the prbtest_rbdata structure is declared like this:
> 
>     53  /* test data structure */
>     54  struct prbtest_rbdata {
>     55          unsigned int len;
>     56          char text[] __counted_by(len);
>     57  };
> 
> The size of text is not really counted by len, it's "MAX_RBDATA_TEXT_SIZE
> + 1".  The condition "if (dat->text[dat->len] != '\0')" is reading one
> element beyond the __counted_by() value so something should complain if
> we enable all the debugging, right?

You are right, we are reading past the __counted_by().
But I don't get any complains with CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y
on either clang or gcc.
We could remove the __counted_by, but I assume somebody will try to add it back
at some point.
Or we account for the terminator in dat->len:

diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c
index ef4a2beea57a..106f4c7ffc86 100644
--- a/kernel/printk/printk_ringbuffer_kunit_test.c
+++ b/kernel/printk/printk_ringbuffer_kunit_test.c
@@ -85,14 +85,15 @@ static bool prbtest_check_data(const struct prbtest_rbdata *dat)
        unsigned int len;
 
        /* Sane length? */
-       if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
+       if (dat->len < 2 || dat->len > MAX_RBDATA_TEXT_SIZE + 1)
                return false;
 
-       if (dat->text[dat->len] != '\0')
+       len = dat->len - 1;
+
+       if (dat->text[len] != '\0')
                return false;
 
        /* String repeats with the same character? */
-       len = dat->len;
        while (len--) {
                if (dat->text[len] != dat->text[0])
                        return false;
@@ -114,10 +115,9 @@ static int prbtest_writer(void *data)
        kunit_info(tr->test_data->test, "start thread %03lu (writer)\n", tr->num);
 
        for (;;) {
-               /* ensure at least 1 character */
-               text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE);
-               /* +1 for terminator. */
-               record_size = sizeof(struct prbtest_rbdata) + text_size + 1;
+               /* ensure at least 1 character, +1 for terminator */
+               text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE) + 1;
+               record_size = sizeof(struct prbtest_rbdata) + text_size;
                WARN_ON_ONCE(record_size > MAX_PRB_RECORD_SIZE);
 
                /* specify the text sizes for reservation */
@@ -142,7 +142,7 @@ static int prbtest_writer(void *data)
                        dat = (struct prbtest_rbdata *)r.text_buf;
                        dat->len = text_size;
                        memset(dat->text, text_id, text_size);
-                       dat->text[text_size] = 0;
+                       dat->text[text_size - 1] = '\0';
 
                        prb_commit(&e);


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [bug report] printk: ringbuffer: Add KUnit test
  2025-06-26  6:59 ` Thomas Weißschuh
@ 2025-07-01 15:23   ` Dan Carpenter
  2025-07-01 15:56   ` Petr Mladek
  2025-07-02 20:22   ` Nathan Chancellor
  2 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2025-07-01 15:23 UTC (permalink / raw)
  To: Thomas Weißschuh, Nathan Chancellor
  Cc: John Ogness, Kees Cook, linux-hardening, Petr Mladek

Let me add Nathan to the CC list.  He knows a lot about __counted_by().
The dat->text is __counted_by() dat->len so the line:

	if (dat->text[dat->len] != '\0')
		return false;

Should have generated a warning of some sort, right?  I'm including
the whole thread even though I assume you are huge fan of b4.

regards,
dan carpenter

On Thu, Jun 26, 2025 at 08:59:52AM +0200, Thomas Weißschuh wrote:
> On Wed, Jun 25, 2025 at 10:22:19AM -0500, Dan Carpenter wrote:
> > Hello Thomas Weißschuh,
> > 
> > The patch 5ea2bcdfbf46: "printk: ringbuffer: Add KUnit test" from Jun
> > 12, 2025, leads to the following static checker warning:
> > 
> > 	kernel/printk/printk_ringbuffer_kunit_test.c:91 prbtest_check_data()
> > 	(unpublished script worries this an off by one)
> > 
> > kernel/printk/printk_ringbuffer_kunit_test.c
> >     83 static bool prbtest_check_data(const struct prbtest_rbdata *dat)
> >     84 {
> >     85 	unsigned int len;
> >     86 
> >     87 	/* Sane length? */
> >     88 	if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
> >     89 		return false;
> >     90 
> > --> 91 	if (dat->text[dat->len] != '\0')
> >     92 		return false;
> >     93 
> > 
> > My question is that the prbtest_rbdata structure is declared like this:
> > 
> >     53  /* test data structure */
> >     54  struct prbtest_rbdata {
> >     55          unsigned int len;
> >     56          char text[] __counted_by(len);
> >     57  };
> > 
> > The size of text is not really counted by len, it's "MAX_RBDATA_TEXT_SIZE
> > + 1".  The condition "if (dat->text[dat->len] != '\0')" is reading one
> > element beyond the __counted_by() value so something should complain if
> > we enable all the debugging, right?
> 
> You are right, we are reading past the __counted_by().
> But I don't get any complains with CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y
> on either clang or gcc.
> We could remove the __counted_by, but I assume somebody will try to add it back
> at some point.
> Or we account for the terminator in dat->len:
> 
> diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c
> index ef4a2beea57a..106f4c7ffc86 100644
> --- a/kernel/printk/printk_ringbuffer_kunit_test.c
> +++ b/kernel/printk/printk_ringbuffer_kunit_test.c
> @@ -85,14 +85,15 @@ static bool prbtest_check_data(const struct prbtest_rbdata *dat)
>         unsigned int len;
>  
>         /* Sane length? */
> -       if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
> +       if (dat->len < 2 || dat->len > MAX_RBDATA_TEXT_SIZE + 1)
>                 return false;
>  
> -       if (dat->text[dat->len] != '\0')
> +       len = dat->len - 1;
> +
> +       if (dat->text[len] != '\0')
>                 return false;
>  
>         /* String repeats with the same character? */
> -       len = dat->len;
>         while (len--) {
>                 if (dat->text[len] != dat->text[0])
>                         return false;
> @@ -114,10 +115,9 @@ static int prbtest_writer(void *data)
>         kunit_info(tr->test_data->test, "start thread %03lu (writer)\n", tr->num);
>  
>         for (;;) {
> -               /* ensure at least 1 character */
> -               text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE);
> -               /* +1 for terminator. */
> -               record_size = sizeof(struct prbtest_rbdata) + text_size + 1;
> +               /* ensure at least 1 character, +1 for terminator */
> +               text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE) + 1;
> +               record_size = sizeof(struct prbtest_rbdata) + text_size;
>                 WARN_ON_ONCE(record_size > MAX_PRB_RECORD_SIZE);
>  
>                 /* specify the text sizes for reservation */
> @@ -142,7 +142,7 @@ static int prbtest_writer(void *data)
>                         dat = (struct prbtest_rbdata *)r.text_buf;
>                         dat->len = text_size;
>                         memset(dat->text, text_id, text_size);
> -                       dat->text[text_size] = 0;
> +                       dat->text[text_size - 1] = '\0';
>  
>                         prb_commit(&e);

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] printk: ringbuffer: Add KUnit test
  2025-06-26  6:59 ` Thomas Weißschuh
  2025-07-01 15:23   ` Dan Carpenter
@ 2025-07-01 15:56   ` Petr Mladek
  2025-07-02 20:22   ` Nathan Chancellor
  2 siblings, 0 replies; 6+ messages in thread
From: Petr Mladek @ 2025-07-01 15:56 UTC (permalink / raw)
  To: Thomas Weißschuh
  Cc: Dan Carpenter, John Ogness, Kees Cook, linux-hardening

On Thu 2025-06-26 08:59:52, Thomas Weißschuh wrote:
> On Wed, Jun 25, 2025 at 10:22:19AM -0500, Dan Carpenter wrote:
> > Hello Thomas Weißschuh,
> > 
> > The patch 5ea2bcdfbf46: "printk: ringbuffer: Add KUnit test" from Jun
> > 12, 2025, leads to the following static checker warning:
> > 
> > 	kernel/printk/printk_ringbuffer_kunit_test.c:91 prbtest_check_data()
> > 	(unpublished script worries this an off by one)
> > 
> > kernel/printk/printk_ringbuffer_kunit_test.c
> >     83 static bool prbtest_check_data(const struct prbtest_rbdata *dat)
> >     84 {
> >     85 	unsigned int len;
> >     86 
> >     87 	/* Sane length? */
> >     88 	if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
> >     89 		return false;
> >     90 
> > --> 91 	if (dat->text[dat->len] != '\0')
> >     92 		return false;
> >     93 
> > 
> > My question is that the prbtest_rbdata structure is declared like this:
> > 
> >     53  /* test data structure */
> >     54  struct prbtest_rbdata {
> >     55          unsigned int len;
> >     56          char text[] __counted_by(len);
> >     57  };
> > 
> > The size of text is not really counted by len, it's "MAX_RBDATA_TEXT_SIZE
> > + 1".  The condition "if (dat->text[dat->len] != '\0')" is reading one
> > element beyond the __counted_by() value so something should complain if
> > we enable all the debugging, right?
> 
> You are right, we are reading past the __counted_by().
> But I don't get any complains with CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y
> on either clang or gcc.
> We could remove the __counted_by, but I assume somebody will try to add it back
> at some point.
> Or we account for the terminator in dat->len:

It means that the value will be the size occupied by the string
including the trailing '\0'.

It means that we need to rename it, for example, len -> size.
Because using "len" for size is confusing and error prone.
See below.

> diff --git a/kernel/printk/printk_ringbuffer_kunit_test.c b/kernel/printk/printk_ringbuffer_kunit_test.c
> index ef4a2beea57a..106f4c7ffc86 100644
> --- a/kernel/printk/printk_ringbuffer_kunit_test.c
> +++ b/kernel/printk/printk_ringbuffer_kunit_test.c
> @@ -85,14 +85,15 @@ static bool prbtest_check_data(const struct prbtest_rbdata *dat)
>         unsigned int len;
>  
>         /* Sane length? */
> -       if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
> +       if (dat->len < 2 || dat->len > MAX_RBDATA_TEXT_SIZE + 1)
>                 return false;
>  
> -       if (dat->text[dat->len] != '\0')
> +       len = dat->len - 1;

This is one example, where it just looks just ugly.

> +
> +       if (dat->text[len] != '\0')
>                 return false;
>  
>         /* String repeats with the same character? */
> -       len = dat->len;
>         while (len--) {
>                 if (dat->text[len] != dat->text[0])
>                         return false;
> @@ -114,10 +115,9 @@ static int prbtest_writer(void *data)
>         kunit_info(tr->test_data->test, "start thread %03lu (writer)\n", tr->num);
>  
>         for (;;) {
> -               /* ensure at least 1 character */
> -               text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE);
> -               /* +1 for terminator. */
> -               record_size = sizeof(struct prbtest_rbdata) + text_size + 1;
> +               /* ensure at least 1 character, +1 for terminator */
> +               text_size = get_random_u32_inclusive(1, MAX_RBDATA_TEXT_SIZE) + 1;

This is where the naming goes beyond sanity. We allow to break the
limit by setting "text_size" to MAX_RBDATA_TEXT_SIZE + 1.

It is because MAX_RBDATA_TEXT_SIZE is used to limit the length of
the string (without the trailing '\0'). Huh.

> +               record_size = sizeof(struct prbtest_rbdata) + text_size;
>                 WARN_ON_ONCE(record_size > MAX_PRB_RECORD_SIZE);
>  
>                 /* specify the text sizes for reservation */
> @@ -142,7 +142,7 @@ static int prbtest_writer(void *data)
>                         dat = (struct prbtest_rbdata *)r.text_buf;
>                         dat->len = text_size;
>                         memset(dat->text, text_id, text_size);
> -                       dat->text[text_size] = 0;
> +                       dat->text[text_size - 1] = '\0';
>  
>                         prb_commit(&e);

This patch forgot to update prbtest_fail_record(). It limits the
printed string by dat->len. But the value newly counts the trailing
'\0'.

OK, I am going to send a patch with sane names where all this
should be fixed.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] printk: ringbuffer: Add KUnit test
  2025-06-26  6:59 ` Thomas Weißschuh
  2025-07-01 15:23   ` Dan Carpenter
  2025-07-01 15:56   ` Petr Mladek
@ 2025-07-02 20:22   ` Nathan Chancellor
  2025-07-02 20:33     ` Dan Carpenter
  2 siblings, 1 reply; 6+ messages in thread
From: Nathan Chancellor @ 2025-07-02 20:22 UTC (permalink / raw)
  To: Thomas Weißschuh
  Cc: Dan Carpenter, John Ogness, Kees Cook, linux-hardening,
	Petr Mladek

On Thu, Jun 26, 2025 at 08:59:52AM +0200, Thomas Weißschuh wrote:
> On Wed, Jun 25, 2025 at 10:22:19AM -0500, Dan Carpenter wrote:
> > Hello Thomas Weißschuh,
> > 
> > The patch 5ea2bcdfbf46: "printk: ringbuffer: Add KUnit test" from Jun
> > 12, 2025, leads to the following static checker warning:
> > 
> > 	kernel/printk/printk_ringbuffer_kunit_test.c:91 prbtest_check_data()
> > 	(unpublished script worries this an off by one)
> > 
> > kernel/printk/printk_ringbuffer_kunit_test.c
> >     83 static bool prbtest_check_data(const struct prbtest_rbdata *dat)
> >     84 {
> >     85 	unsigned int len;
> >     86 
> >     87 	/* Sane length? */
> >     88 	if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
> >     89 		return false;
> >     90 
> > --> 91 	if (dat->text[dat->len] != '\0')
> >     92 		return false;
> >     93 
> > 
> > My question is that the prbtest_rbdata structure is declared like this:
> > 
> >     53  /* test data structure */
> >     54  struct prbtest_rbdata {
> >     55          unsigned int len;
> >     56          char text[] __counted_by(len);
> >     57  };
> > 
> > The size of text is not really counted by len, it's "MAX_RBDATA_TEXT_SIZE
> > + 1".  The condition "if (dat->text[dat->len] != '\0')" is reading one
> > element beyond the __counted_by() value so something should complain if
> > we enable all the debugging, right?
> 
> You are right, we are reading past the __counted_by().
> But I don't get any complains with CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y
> on either clang or gcc.

You will need CONFIG_UBSAN_BOUNDS to see warnings from __counted_by()
but they are present if I enable it with this test:

  [    0.507904] ------------[ cut here ]------------
  [    0.507904] UBSAN: array-index-out-of-bounds in kernel/printk/printk_ringbuffer_kunit_test.c:132:4
  [    0.507907] index 108 is out of range for type 'char[] __counted_by(len)' (aka 'char[]')
  [    0.507909] CPU: 3 UID: 0 PID: 392 Comm: prbtest writer  Tainted: G                 N  6.16.0-rc4-next-20250702 #1 PREEMPT(voluntary)
  [    0.507912] Tainted: [N]=TEST
  [    0.507913] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014
  [    0.507914] Call Trace:
  [    0.507916]  <TASK>
  [    0.507918]  dump_stack_lvl+0x84/0xc0
  [    0.507922]  ubsan_epilogue+0x5/0x30
  [    0.507925]  __ubsan_handle_out_of_bounds+0xa2/0xc0
  [    0.507928]  prbtest_writer+0x16a/0x210
  [    0.507932]  ? __pfx_prbtest_writer+0x10/0x10
  [    0.507934]  kthread+0x21a/0x260
  [    0.507937]  ? __pfx_kthread+0x10/0x10
  [    0.507938]  ret_from_fork+0x77/0xd0
  [    0.507940]  ? __pfx_kthread+0x10/0x10
  [    0.507941]  ret_from_fork_asm+0x1a/0x30
  [    0.507945]  </TASK>
  [    0.507945] ---[ end trace ]---

  [    0.516113] ------------[ cut here ]------------
  [    0.516115] UBSAN: array-index-out-of-bounds in kernel/printk/printk_ringbuffer_kunit_test.c:91:6
  [    0.516117]     # test_readerwriter: lib/ubsan.c:228: array-index-out-of-bounds in kernel/printk/printk_ringbuffer_kunit_test.c
  [    0.516119] index 126 is out of range for type 'char const[] __counted_by(len)' (aka 'const char[]')
  [    0.516122] CPU: 0 UID: 0 PID: 389 Comm: kunit_try_catch Tainted: G                 N  6.16.0-rc4-next-20250702 #1 PREEMPT(voluntary)
  [    0.516125] Tainted: [N]=TEST
  [    0.516126] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014
  [    0.516127] Call Trace:
  [    0.516129]  <TASK>
  [    0.516131]  dump_stack_lvl+0x84/0xc0
  [    0.516136]  ubsan_epilogue+0x5/0x30
  [    0.516138]  __ubsan_handle_out_of_bounds+0xa2/0xc0
  [    0.516142]  prbtest_reader+0x2ff/0x480
  [    0.516145]  ? srso_alias_return_thunk+0x5/0xfbef5
  [    0.516147]  ? set_next_entity+0x5e/0x140
  [    0.516150]  ? __pfx_prbtest_wakeup_callback+0x10/0x10
  [    0.516155]  ? set_cpus_allowed_ptr+0x83/0xb0
  [    0.516158]  test_readerwriter+0x297/0x3c0
  [    0.516161]  kunit_try_run_case+0x8c/0x190
  [    0.516164]  kunit_generic_run_threadfn_adapter+0x1a/0x40
  [    0.516166]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
  [    0.516167]  kthread+0x21a/0x260
  [    0.516169]  ? __pfx_kthread+0x10/0x10
  [    0.516170]  ret_from_fork+0x77/0xd0
  [    0.516173]  ? __pfx_kthread+0x10/0x10
  [    0.516174]  ret_from_fork_asm+0x1a/0x30
  [    0.516177]  </TASK>
  [    0.516178] ---[ end trace ]---

I see Petr sent an updated patch for this, which does resolve the
warning in my testing.

https://lore.kernel.org/20250702095157.110916-4-pmladek@suse.com/

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] printk: ringbuffer: Add KUnit test
  2025-07-02 20:22   ` Nathan Chancellor
@ 2025-07-02 20:33     ` Dan Carpenter
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2025-07-02 20:33 UTC (permalink / raw)
  To: Nathan Chancellor
  Cc: Thomas Weißschuh, John Ogness, Kees Cook, linux-hardening,
	Petr Mladek

On Wed, Jul 02, 2025 at 01:22:03PM -0700, Nathan Chancellor wrote:
> On Thu, Jun 26, 2025 at 08:59:52AM +0200, Thomas Weißschuh wrote:
> > On Wed, Jun 25, 2025 at 10:22:19AM -0500, Dan Carpenter wrote:
> > > Hello Thomas Weißschuh,
> > > 
> > > The patch 5ea2bcdfbf46: "printk: ringbuffer: Add KUnit test" from Jun
> > > 12, 2025, leads to the following static checker warning:
> > > 
> > > 	kernel/printk/printk_ringbuffer_kunit_test.c:91 prbtest_check_data()
> > > 	(unpublished script worries this an off by one)
> > > 
> > > kernel/printk/printk_ringbuffer_kunit_test.c
> > >     83 static bool prbtest_check_data(const struct prbtest_rbdata *dat)
> > >     84 {
> > >     85 	unsigned int len;
> > >     86 
> > >     87 	/* Sane length? */
> > >     88 	if (dat->len < 1 || dat->len > MAX_RBDATA_TEXT_SIZE)
> > >     89 		return false;
> > >     90 
> > > --> 91 	if (dat->text[dat->len] != '\0')
> > >     92 		return false;
> > >     93 
> > > 
> > > My question is that the prbtest_rbdata structure is declared like this:
> > > 
> > >     53  /* test data structure */
> > >     54  struct prbtest_rbdata {
> > >     55          unsigned int len;
> > >     56          char text[] __counted_by(len);
> > >     57  };
> > > 
> > > The size of text is not really counted by len, it's "MAX_RBDATA_TEXT_SIZE
> > > + 1".  The condition "if (dat->text[dat->len] != '\0')" is reading one
> > > element beyond the __counted_by() value so something should complain if
> > > we enable all the debugging, right?
> > 
> > You are right, we are reading past the __counted_by().
> > But I don't get any complains with CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y
> > on either clang or gcc.
> 
> You will need CONFIG_UBSAN_BOUNDS to see warnings from __counted_by()
> but they are present if I enable it with this test:
> 
>   [    0.507904] ------------[ cut here ]------------
>   [    0.507904] UBSAN: array-index-out-of-bounds in kernel/printk/printk_ringbuffer_kunit_test.c:132:4

Thanks, Nathan.  You are the best.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-07-02 20:33 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-25 15:22 [bug report] printk: ringbuffer: Add KUnit test Dan Carpenter
2025-06-26  6:59 ` Thomas Weißschuh
2025-07-01 15:23   ` Dan Carpenter
2025-07-01 15:56   ` Petr Mladek
2025-07-02 20:22   ` Nathan Chancellor
2025-07-02 20:33     ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.