[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