Re: /init/main.c issue

[Povl Ole Haarlev Olsen <debian-security@stderr.dk>]

On Mon, 2 Jan 2017, Sticky Chocolate wrote:
> I found a issue with some functions in /init/main.c of the linux kernel.
> They all involve the problem of the use of strcpy(I found that 3 functions
> use strcpy) . I thought maby this could lead to buffer overflow. Im not
> completely sure

Well, let's do a quick analysis...

> __________________________________________________________________________________________________
>
> main.c:ln 839,col 59
> static void __init do_initcall_level(int level)
> {
>    initcall_t *fn;
>
>    strcpy(initcall_command_line, saved_command_line);

initcall_command_line is defined as a static char * and saved_command_line 
is defined as a char *. At this point we don't know how much space is 
allocated for either of them, but I'll get back to this one line in a bit.

>    parse_args(initcall_level_names[level],
>           initcall_command_line, __start___param,
>           __stop___param - __start___param,
>           level, level,
>           NULL, &repair_env_string);
>
>    for (fn = initcall_levels[level]; fn < initcall_levels[level+1]; fn++)
>        do_one_initcall(*fn);
> }
> __________________________________________________________________________________________________
>
> line:ln 699,col 55
>
> static int __init initcall_blacklist(char *str)
> {
>    char *str_entry;
>    struct blacklist_entry *entry;
>
>    /* str argument is a comma-separated list of functions */
>    do {
>        str_entry = strsep(&str, ",");
>        if (str_entry) {
>            pr_debug("blacklisting initcall %s\n", str_entry);
>            entry = alloc_bootmem(sizeof(*entry));
>            entry->buf = alloc_bootmem(strlen(str_entry) + 1);
>            strcpy(entry->buf, str_entry);

First strlen(str_entry) + 1 bytes are allocated for entry->buf (in other 
words enough bytes for str_entry and the following '\0') and then 
str_entry is copied to entry->buf, which is obviously large enough since 
we just allocated enough bytes for str_entry and the '\0'. No problem 
here...

>            list_add(&entry->next, &blacklisted_initcalls);
>        }
>    } while (str_entry);
>
>    return 0;
> }
> ____________________________________________________________________________________________________________________________________________________
>
> line:ln 368,ln 369,col 55, col 51
>
> static void __init setup_command_line(char *command_line)
> {
>    saved_command_line =
>        memblock_virt_alloc(strlen(boot_command_line) + 1, 0);
>    initcall_command_line =
>        memblock_virt_alloc(strlen(boot_command_line) + 1, 0);
>    static_command_line = memblock_virt_alloc(strlen(command_line) + 1, 0);
>    strcpy(saved_command_line, boot_command_line);
>    strcpy(static_command_line, command_line);

strlen(boot_command_line) + 1 bytes are allocated for saved_command_line 
and initcall_command_line. In other words, they are the same size. That 
will be important in a bit.

strlen(command_line) + 1 bytes are allocated for static_command_line.

boot_coomand_line is copied to saved_command_line, which we just allocated 
enough space for. No problem here...

command_line is copied to static_command_line. Again, we just allocated 
enough space for this. No problem here either...

Ok, back to saved_command_line, initcall_command_line and the first 
strcpy() you mentioned:

>    strcpy(initcall_command_line, saved_command_line);

Since saved_command_line and initcall_command_line are the same size, a 
string that fits in saved_command_line will also fit in 
initcall_command_line, so no problem there either. (Unless one of them are 
reallocated at another point in the code. I haven't checked that.)

> }

TL;DR: No problems as far as I can tell...

--
Povl Ole