[LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Li Wang]

When the test is run with a kernel booting with many parameters, the
buffer size is often not large enough to store the complete command
line. This results in a buffer overflow and the test complains with
the following message:

  tst_kconfig.c:609: TWARN: Buffer overflowed while parsing /proc/cmdline

Note:

Petr point out that these configs, which are generated by toolchain will
be longer than 128 chars someday, but I don't think that is the reason
we need raise our parsed buffer, since tst_kcmdline_parse() was just add
for popular parameter (which always pass by user and short). So far I
don't see any LTP test parse a longer parameters.

Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")
Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Petr Vorel <pvorel@suse.cz>
---
 include/tst_kconfig.h | 2 +-
 lib/tst_kconfig.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h
index dcb370574..23f807409 100644
--- a/include/tst_kconfig.h
+++ b/include/tst_kconfig.h
@@ -87,7 +87,7 @@ int tst_kconfig_check(const char *const kconfigs[]);
  */
 struct tst_kcmdline_var {
 	const char *key;
-	char value[128];
+	char value[256];
 	bool found;
 };
 
diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c
index e16ca1400..8eb1b803f 100644
--- a/lib/tst_kconfig.c
+++ b/lib/tst_kconfig.c
@@ -569,7 +569,7 @@ char tst_kconfig_get(const char *confname)
 
 void tst_kcmdline_parse(struct tst_kcmdline_var params[], size_t params_len)
 {
-	char buf[128], line[512];
+	char buf[256], line[512];
 	size_t b_pos = 0,l_pos =0, i;
 	int var_id = -1;
 
@@ -606,7 +606,7 @@ void tst_kcmdline_parse(struct tst_kcmdline_var params[], size_t params_len)
 		break;
 		default:
 			if (b_pos + 1 >= sizeof(buf)) {
-				tst_res(TWARN, "Buffer overflowed while parsing /proc/cmdline");
+				tst_res(TINFO, "Buffer overflowed while parsing /proc/cmdline");
 				while (line[l_pos] != '\0' && line[l_pos] != ' ' && line[l_pos] != '\n')
 					l_pos++;
 
-- 
2.45.2


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Li Wang]

 Li Wang <liwang@redhat.com> wrote:

When the test is run with a kernel booting with many parameters, the
> buffer size is often not large enough to store the complete command
> line. This results in a buffer overflow and the test complains with
> the following message:
>
>   tst_kconfig.c:609: TWARN: Buffer overflowed while parsing /proc/cmdline
>
> Note:
>
> Petr point out that these configs, which are generated by toolchain will
> be longer than 128 chars someday, but I don't think that is the reason
> we need raise our parsed buffer, since tst_kcmdline_parse() was just add
> for popular parameter (which always pass by user and short). So far I
> don't see any LTP test parse a longer parameters.
>
> Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Petr Vorel <pvorel@suse.cz>
> ---
>  include/tst_kconfig.h | 2 +-
>  lib/tst_kconfig.c     | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h
> index dcb370574..23f807409 100644
> --- a/include/tst_kconfig.h
> +++ b/include/tst_kconfig.h
> @@ -87,7 +87,7 @@ int tst_kconfig_check(const char *const kconfigs[]);
>   */
>  struct tst_kcmdline_var {
>         const char *key;
> -       char value[128];
> +       char value[256];
>         bool found;
>  };
>
> diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c
> index e16ca1400..8eb1b803f 100644
> --- a/lib/tst_kconfig.c
> +++ b/lib/tst_kconfig.c
> @@ -569,7 +569,7 @@ char tst_kconfig_get(const char *confname)
>
>  void tst_kcmdline_parse(struct tst_kcmdline_var params[], size_t
> params_len)
>  {
> -       char buf[128], line[512];
> +       char buf[256], line[512];
>

Petr, if you are still worried, feel free to enlarge them to char
'buf[512], line[2048];' during merge :).

BTW, I don't want to allocate the size dynamically to make the code more
complicated.


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Petr Vorel]

>  Li Wang <liwang@redhat.com> wrote:

> When the test is run with a kernel booting with many parameters, the
> > buffer size is often not large enough to store the complete command
> > line. This results in a buffer overflow and the test complains with
> > the following message:

> >   tst_kconfig.c:609: TWARN: Buffer overflowed while parsing /proc/cmdline

> > Note:

> > Petr point out that these configs, which are generated by toolchain will
> > be longer than 128 chars someday, but I don't think that is the reason
> > we need raise our parsed buffer, since tst_kcmdline_parse() was just add
> > for popular parameter (which always pass by user and short). So far I
> > don't see any LTP test parse a longer parameters.

> > Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > Cc: Petr Vorel <pvorel@suse.cz>
> > ---
> >  include/tst_kconfig.h | 2 +-
> >  lib/tst_kconfig.c     | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)

> > diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h
> > index dcb370574..23f807409 100644
> > --- a/include/tst_kconfig.h
> > +++ b/include/tst_kconfig.h
> > @@ -87,7 +87,7 @@ int tst_kconfig_check(const char *const kconfigs[]);
> >   */
> >  struct tst_kcmdline_var {
> >         const char *key;
> > -       char value[128];
> > +       char value[256];
> >         bool found;
> >  };

> > diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c
> > index e16ca1400..8eb1b803f 100644
> > --- a/lib/tst_kconfig.c
> > +++ b/lib/tst_kconfig.c
> > @@ -569,7 +569,7 @@ char tst_kconfig_get(const char *confname)

> >  void tst_kcmdline_parse(struct tst_kcmdline_var params[], size_t
> > params_len)
> >  {
> > -       char buf[128], line[512];
> > +       char buf[256], line[512];


> Petr, if you are still worried, feel free to enlarge them to char
> 'buf[512], line[2048];' during merge :).

Li, I'm ok with 256 :).
Reviewed-by: Petr Vorel <pvorel@suse.cz>

-				tst_res(TWARN, "Buffer overflowed while parsing /proc/cmdline");
+				tst_res(TINFO, "Buffer overflowed while parsing /proc/cmdline");

But I wonder if we should keep TWARN. Or at least add
+				tst_res(TINFO, "WARNING: Buffer overflowed while parsing /proc/cmdline");

BTW I also see Cyril's comment about adding TINFO | TWARN (or TINFO_WARN), where
he does not like neither of these two and even suggest to actually remove TWARN.
https://lore.kernel.org/ltp/ZldFa-3CXXbVKmVW@yuki/

> BTW, I don't want to allocate the size dynamically to make the code more
> complicated.

Fully agree.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Li Wang]

On Thu, Jun 20, 2024 at 1:21 PM Petr Vorel <pvorel@suse.cz> wrote:

> >  Li Wang <liwang@redhat.com> wrote:
>
> > When the test is run with a kernel booting with many parameters, the
> > > buffer size is often not large enough to store the complete command
> > > line. This results in a buffer overflow and the test complains with
> > > the following message:
>
> > >   tst_kconfig.c:609: TWARN: Buffer overflowed while parsing
> /proc/cmdline
>
> > > Note:
>
> > > Petr point out that these configs, which are generated by toolchain
> will
> > > be longer than 128 chars someday, but I don't think that is the reason
> > > we need raise our parsed buffer, since tst_kcmdline_parse() was just
> add
> > > for popular parameter (which always pass by user and short). So far I
> > > don't see any LTP test parse a longer parameters.
>
> > > Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")
> > > Signed-off-by: Li Wang <liwang@redhat.com>
> > > Cc: Petr Vorel <pvorel@suse.cz>
> > > ---
> > >  include/tst_kconfig.h | 2 +-
> > >  lib/tst_kconfig.c     | 4 ++--
> > >  2 files changed, 3 insertions(+), 3 deletions(-)
>
> > > diff --git a/include/tst_kconfig.h b/include/tst_kconfig.h
> > > index dcb370574..23f807409 100644
> > > --- a/include/tst_kconfig.h
> > > +++ b/include/tst_kconfig.h
> > > @@ -87,7 +87,7 @@ int tst_kconfig_check(const char *const kconfigs[]);
> > >   */
> > >  struct tst_kcmdline_var {
> > >         const char *key;
> > > -       char value[128];
> > > +       char value[256];
> > >         bool found;
> > >  };
>
> > > diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c
> > > index e16ca1400..8eb1b803f 100644
> > > --- a/lib/tst_kconfig.c
> > > +++ b/lib/tst_kconfig.c
> > > @@ -569,7 +569,7 @@ char tst_kconfig_get(const char *confname)
>
> > >  void tst_kcmdline_parse(struct tst_kcmdline_var params[], size_t
> > > params_len)
> > >  {
> > > -       char buf[128], line[512];
> > > +       char buf[256], line[512];
>
>
> > Petr, if you are still worried, feel free to enlarge them to char
> > 'buf[512], line[2048];' during merge :).
>
> Li, I'm ok with 256 :).
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>

Thanks.


> -                               tst_res(TWARN, "Buffer overflowed while
> parsing /proc/cmdline");
> +                               tst_res(TINFO, "Buffer overflowed while
> parsing /proc/cmdline");
>
>

> But I wonder if we should keep TWARN. Or at least add
> +                               tst_res(TINFO, "WARNING: Buffer overflowed
> while parsing /proc/cmdline");
>

+1, I'd merge like this if Cyril has no more comments.


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Li Wang]

Hi All,

Li Wang <liwang@redhat.com> wrote:


> > > diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c
>> > > index e16ca1400..8eb1b803f 100644
>> > > --- a/lib/tst_kconfig.c
>> > > +++ b/lib/tst_kconfig.c
>> > > @@ -569,7 +569,7 @@ char tst_kconfig_get(const char *confname)
>>
>> > >  void tst_kcmdline_parse(struct tst_kcmdline_var params[], size_t
>> > > params_len)
>> > >  {
>> > > -       char buf[128], line[512];
>> > > +       char buf[256], line[512];
>>
>>
>> > Petr, if you are still worried, feel free to enlarge them to char
>> > 'buf[512], line[2048];' during merge :).
>>
>> Li, I'm ok with 256 :).
>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>>
>
> Thanks.
>
>
>> -                               tst_res(TWARN, "Buffer overflowed while
>> parsing /proc/cmdline");
>> +                               tst_res(TINFO, "Buffer overflowed while
>> parsing /proc/cmdline");
>>
>>
>
>> But I wonder if we should keep TWARN. Or at least add
>> +                               tst_res(TINFO, "WARNING: Buffer
>> overflowed while parsing /proc/cmdline");
>>
>
> +1, I'd merge like this if Cyril has no more comments.
>

I dared to merge this patch to avoid annoying our engineers with this
little WARNING issue.
Thanks!

-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Petr Vorel]

Hi Li,

...
> I dared to merge this patch to avoid annoying our engineers with this
> little WARNING issue.
> Thanks!

Sure, thanks for proceeding (no worth of blocking).
I'll add macros to my TODO list, working on it sometimes during July.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Li Wang]

Hi Petr, All,


> I'll add macros to my TODO list, working on it sometimes during July.
>

The tricky part may be for TFAIL (TBROK) to distinguish from TERRNO or
TTERRNO in one macro.

  tst_res(TFAIL, " ")
  tst_res(TFAIL | TTERRNO, " ")
  tst_res(TFAIL | TERRNO, " ");

I haven't got a better idea unless we allow creating more macros like:

 TST_RES_TFAIL()
 TST_RES_TFAIL_TTERRNO()
 TST_RES_TFAIL_TERRNO()

But TBH, this looks a bit messy and not good for memories.


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2] tst_kconfig: Avoid reporting buffer overflow when parsing /proc/cmdline

[Petr Vorel]

> Hi Petr, All,


> > I'll add macros to my TODO list, working on it sometimes during July.


> The tricky part may be for TFAIL (TBROK) to distinguish from TERRNO or
> TTERRNO in one macro.

>   tst_res(TFAIL, " ")
>   tst_res(TFAIL | TTERRNO, " ")
>   tst_res(TFAIL | TERRNO, " ");

> I haven't got a better idea unless we allow creating more macros like:

>  TST_RES_TFAIL()
>  TST_RES_TFAIL_TTERRNO()
>  TST_RES_TFAIL_TERRNO()

> But TBH, this looks a bit messy and not good for memories.

Yeah :(. And I don't see any good name either.

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp