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

[LTP] [PATCH] tst_kconfig: Avoid 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

Signed-off-by: Li Wang <liwang@redhat.com>
---
 lib/tst_kconfig.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/tst_kconfig.c b/lib/tst_kconfig.c
index e16ca1400..be3842c2d 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[512], line[512];
 	size_t b_pos = 0,l_pos =0, i;
 	int var_id = -1;
 
-- 
2.45.2


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

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

[Petr Vorel]

> 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

Thanks for the fix!

Reviewed-by: Petr Vorel <pvorel@suse.cz>
Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")

NOTE tst_kconfig_read() has char line[128], also struct tst_kcmdline_var member
has this length. The longest line on some on my systems is 109, it's still OK,
hopefully it stays :).

Kind regards,
Petr

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

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

[Li Wang]

On Wed, Jun 19, 2024 at 3:47 PM Petr Vorel <pvorel@suse.cz> 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
>
> Thanks for the fix!
>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")
>
> NOTE tst_kconfig_read() has char line[128], also struct tst_kcmdline_var
> member
>

Typically 128 is long enough for Linux kernel parameters, otherwise it hard
for people to memorize the name and value.



> has this length. The longest line on some on my systems is 109, it's still
> OK,
> hopefully it stays :).
>

Hmm, good point. After thinking it over, seems too hasty to enlarge the
buf[] size to 512.

We'd better keep the size same between 'tst_kcmdline_var.value' to the
local 'buf[]'.
And the overflow is not a problem, it just drops some unused info[1] to
reloop to
the head for saving our target parameter.

[1] BOOT_IMAGE=(hd0,gpt2)/ostree/centos-dd7415ed2c7cc3f65bdc4bf8f9a63b95bbb13ee0fee633f6b92a872944d1d6e4/vmlinuz-5.14.0-4xx.4xx.4495_1336583944.el9iv.x86_64

I'm thinking of lowering the priority info from TWARN to TINFO in that line:
    tst_res(TWARN, "Buffer overflowed while parsing /proc/cmdline");

I will send a patch V2 once get a better solution.


-- 
Regards,
Li Wang

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

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

[Petr Vorel]

Hi Li,

> On Wed, Jun 19, 2024 at 3:47 PM Petr Vorel <pvorel@suse.cz> 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

> > Thanks for the fix!

> > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")

> > NOTE tst_kconfig_read() has char line[128], also struct tst_kcmdline_var
> > member


> Typically 128 is long enough for Linux kernel parameters, otherwise it hard
> for people to memorize the name and value.

Sure, let's keep it for now, but examples we are getting close :). Sooner or
later these configs, which are generated by toolchain will be longer than 128
chars:

$ awk 'length > max_length { max_length = length; longest_line = $0 } END { print longest_line }' /boot/config*
CONFIG_CC_VERSION_TEXT="gcc (SUSE Linux) 13.2.1 20240206 [revision 67ac78caf31f7cb3202177e6428a46d829b70f23]"

$ awk 'length > max_length { max_length = length; longest_line = $0 } END { print longest_line }' /boot/config* | wc -L
109

I was surprised even real config option was quite long:

$ awk 'length > max_length { max_length = length; longest_line = $0 } END { print longest_line }' /boot/config*
CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf"

$ awk 'length > max_length { max_length = length; longest_line = $0 } END { print longest_line }' /boot/config* | wc -L
97

> > has this length. The longest line on some on my systems is 109, it's still
> > OK,
> > hopefully it stays :).


> Hmm, good point. After thinking it over, seems too hasty to enlarge the
> buf[] size to 512.

> We'd better keep the size same between 'tst_kcmdline_var.value' to the
> local 'buf[]'.
+1. I would even define a constant to keep these 2 sizes the same.

> And the overflow is not a problem, it just drops some unused info[1] to
> reloop to
> the head for saving our target parameter.

> [1] BOOT_IMAGE=(hd0,gpt2)/ostree/centos-dd7415ed2c7cc3f65bdc4bf8f9a63b95bbb13ee0fee633f6b92a872944d1d6e4/vmlinuz-5.14.0-4xx.4xx.4495_1336583944.el9iv.x86_64

Ah, what a long parameter :). OTOH would it harm to enlarge both to 256 or even
512?

> I'm thinking of lowering the priority info from TWARN to TINFO in that line:
>     tst_res(TWARN, "Buffer overflowed while parsing /proc/cmdline");

Yes, but are you sure that occasional long parameter will be always irrelevant?
I'd really increase the size.

Kind regards,
Petr

> I will send a patch V2 once get a better solution.

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

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

[Li Wang]

Hi Petr,

On Wed, Jun 19, 2024 at 9:32 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> > On Wed, Jun 19, 2024 at 3:47 PM Petr Vorel <pvorel@suse.cz> 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
>
> > > Thanks for the fix!
>
> > > Reviewed-by: Petr Vorel <pvorel@suse.cz>
> > > Fixes: 180834982 ("kconfig: add funtion to parse /proc/cmdline")
>
> > > NOTE tst_kconfig_read() has char line[128], also struct
> tst_kcmdline_var
> > > member
>
>
> > Typically 128 is long enough for Linux kernel parameters, otherwise it
> hard
> > for people to memorize the name and value.
>
> Sure, let's keep it for now, but examples we are getting close :). Sooner
> or
> later these configs, which are generated by toolchain will be longer than
> 128
> chars:
>
> $ awk 'length > max_length { max_length = length; longest_line = $0 } END
> { print longest_line }' /boot/config*
> CONFIG_CC_VERSION_TEXT="gcc (SUSE Linux) 13.2.1 20240206 [revision
> 67ac78caf31f7cb3202177e6428a46d829b70f23]"
>
> $ awk 'length > max_length { max_length = length; longest_line = $0 } END
> { print longest_line }' /boot/config* | wc -L
> 109
>
> I was surprised even real config option was quite long:
>
> $ awk 'length > max_length { max_length = length; longest_line = $0 } END
> { print longest_line }' /boot/config*
>
> CONFIG_LSM="landlock,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf"
>
> $ awk 'length > max_length { max_length = length; longest_line = $0 } END
> { print longest_line }' /boot/config* | wc -L
> 97
>
> > > has this length. The longest line on some on my systems is 109, it's
> still
> > > OK,
> > > hopefully it stays :).
>
>
> > Hmm, good point. After thinking it over, seems too hasty to enlarge the
> > buf[] size to 512.
>
> > We'd better keep the size same between 'tst_kcmdline_var.value' to the
> > local 'buf[]'.
> +1. I would even define a constant to keep these 2 sizes the same.
>
> > And the overflow is not a problem, it just drops some unused info[1] to
> > reloop to
> > the head for saving our target parameter.
>
> > [1]
> BOOT_IMAGE=(hd0,gpt2)/ostree/centos-dd7415ed2c7cc3f65bdc4bf8f9a63b95bbb13ee0fee633f6b92a872944d1d6e4/vmlinuz-5.14.0-4xx.4xx.4495_1336583944.el9iv.x86_64
>
> Ah, what a long parameter :). OTOH would it harm to enlarge both to 256 or
> even
> 512?
>

Yes, but to be honest, we always don't care about that long parameter,
and mostly, people just parse something they pass into the kernel,
it is always short and popular.

So I believe raise to 256 is enough size for LTP.



>
> > I'm thinking of lowering the priority info from TWARN to TINFO in that
> line:
> >     tst_res(TWARN, "Buffer overflowed while parsing /proc/cmdline");
>
> Yes, but are you sure that occasional long parameter will be always
> irrelevant?
> I'd really increase the size.
>

I guess yes, at least so far I don't see any LTP test parse long parameters,
it is safe to drop them IMHO.

-- 
Regards,
Li Wang

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