| Message ID | 20240619073345.326108-1-liwang@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | tst_kconfig: Avoid buffer overflow when parsing /proc/cmdline | expand |
> 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
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.
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.
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.
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;
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(-)