Bug 3223

Summary: insmod/modprobe incorrectly parses several parameters in kernel 2.4
Product: Busybox Reporter: Leonid <lly.dev>
Component: OtherAssignee: unassigned
Status: RESOLVED FIXED    
Severity: major CC: busybox-cvs
Priority: P3    
Version: 1.18.x   
Target Milestone: ---   
Hardware: All   
OS: Linux   
Host: Target: linux 2.4
Build:
Attachments: Proposed patch

Description Leonid 2011-02-10 18:57:31 UTC 
Created attachment 2953 [details]
Proposed patch

insmod/modprobe incorrectly parses several char & string parameters in kernel 2.4

Sample output:

# insmod netconsole.o netconsole=@/,@192.168.1.100/
insmod: parameter netconsole requires at least 2 arguments

# modinfo netconsole.o 
filename:       netconsole.o
parm_desc_netconsole: netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]

parm_netconsole:2s
license:        GPL
description:    Network console driver
author:         Maintainer: Herbert P�tzl <herbert@13thfloor.at>
kernel_version: 2.4.37.11


This happens due to "Parse parameter values" loop in modutils-24.c: new_process_module_arguments() wants to recheck ',' delimiter at the end of switch (*pinfo), but it already overwritten by '\0' symbol in code above for 's' & 'c' cases.

Patch attached solves problem for me.
Comment 1 Denys Vlasenko 2011-02-13 03:05:49 UTC 
-                       p = skip_whitespace(p);
-                       if (*p != ',')
-                               break;
+                       if (*p != '\0') {
+                               p = skip_whitespace(p);
+                               if (*p != ',')
+                                       break;
+                       }
                        p = skip_whitespace(p + 1);

And if *p == '\0' because we genuinely reached terminating NUL, not because we replaces ',' with '\0', then what p = skip_whitespace(p + 1) do?
Comment 2 Denys Vlasenko 2011-02-13 03:08:42 UTC 
I propose to save/restore the character, like this:

                                char sv_ch = p[len];
                                p[len] = '\0';
                                obj_string_patch(f, sym->secidx,
                                                 loc - contents, p);
                                loc += tgt_sizeof_char_p;
                                p += len;
                                *p = sv_ch;
Comment 3 Leonid 2011-02-13 17:57:24 UTC 
You are right, my patch breaks some checks.

Anyway, bug fixed & can be closed.
Comment 4 Denys Vlasenko 2011-09-11 17:40:09 UTC 
Fixed in 1.19.x