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

[Petr Vorel <pvorel@suse.cz>]

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