linux-hardening - Re: [bug report] printk: ringbuffer: Add KUnit test

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aGQFIzXtyHw8FAeq@pathway.suse.cz>
Date: Tue, 1 Jul 2025 17:56:19 +0200
From: Petr Mladek <pmladek@...e.com>
To: Thomas Weißschuh <thomas.weissschuh@...utronix.de>
Cc: Dan Carpenter <dan.carpenter@...aro.org>,
	John Ogness <john.ogness@...utronix.de>,
	Kees Cook <kees@...nel.org>, linux-hardening@...r.kernel.org
Subject: Re: [bug report] printk: ringbuffer: Add KUnit test

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