Re: [PATCH] x86, boot: access cmdline when loaded high

[Kees Cook]

On Tue, Jun 11, 2013 at 12:17 PM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Tue, Jun 11, 2013 at 11:41 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> The __cmdline_find_option routine requires that the kernel cmdline be
>> located under 0x10000. When running the compressed boot code, if the
>> cmdline is loaded above this range, it will be ignored. This breaks
>> recognition of things like "earlyprintk=...". Since the compressed boot
>> code is already tricking the cmdline routines about locations, take it all
>> the way and access the cmdline via offset instead of via actual location.
>
> Hi,
>
> What is point of the patch?
> It looks like it try to fix one bug, but it doesn't,
> as there is no bug out there.
>
> Before the patch, command line that is loaded high
> can be processed correctly in arch/x86/boot/compressed/cmdline.c.

It can't. Here is the code from arch/x86/boot/cmdline.c:

int __cmdline_find_option(unsigned long cmdline_ptr, const char
*option, char *buffer, int bufsize)
...
        if (!cmdline_ptr)
                return -1;      /* No command line */

        cptr = cmdline_ptr & 0xf;
        set_fs(cmdline_ptr >> 4);

        while (cptr < 0x10000 && (c = rdfs8(cptr++))) {


This means that if get_cmd_line_ptr() returns an address >= 0x10000,
__cmdline_find_option will simply ignore the cmdline.

>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>> ---
>>  arch/x86/boot/compressed/cmdline.c |   21 +++++++++------------
>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/x86/boot/compressed/cmdline.c b/arch/x86/boot/compressed/cmdline.c
>> index bffd73b..2e39280 100644
>> --- a/arch/x86/boot/compressed/cmdline.c
>> +++ b/arch/x86/boot/compressed/cmdline.c
>> @@ -2,32 +2,29 @@
>>
>>  #ifdef CONFIG_EARLY_PRINTK
>>
>> +/*
>> + * We know where the cmdline is, so just retain it, and use a non-zero
>> + * offset to trick __cmdline_find_option into running.
>> + */
>>  static unsigned long fs;
>>  static inline void set_fs(unsigned long seg)
>>  {
>> -       fs = seg << 4;  /* shift it back */
>> +       fs = real_mode->hdr.cmd_line_ptr;
>> +       fs |= (u64)real_mode->ext_cmd_line_ptr << 32;
>>  }
>>  typedef unsigned long addr_t;
>>  static inline char rdfs8(addr_t addr)
>>  {
>> -       return *((char *)(fs + addr));
>> +       return *((char *)(fs + addr - 0x1));
>>  }
>>  #include "../cmdline.c"
>> -static unsigned long get_cmd_line_ptr(void)
>> -{
>> -       unsigned long cmd_line_ptr = real_mode->hdr.cmd_line_ptr;
>> -
>> -       cmd_line_ptr |= (u64)real_mode->ext_cmd_line_ptr << 32;
>> -
>> -       return cmd_line_ptr;
>> -}
>>  int cmdline_find_option(const char *option, char *buffer, int bufsize)
>>  {
>> -       return __cmdline_find_option(get_cmd_line_ptr(), option, buffer, bufsize);
>> +       return __cmdline_find_option(0x1, option, buffer, bufsize);
>
> that's funny, why do you pick 0x1 here?
> why not use 0x10 or 0?

0 cannot be used because of the !cmdline_ptr test in
__cmdline_find_option. There is no reason it's 1, except that it's the
further possible from 0x10000 without being 0.

-Kees

>
>>  }
>>  int cmdline_find_option_bool(const char *option)
>>  {
>> -       return __cmdline_find_option_bool(get_cmd_line_ptr(), option);
>> +       return __cmdline_find_option_bool(0x1, option);
>>  }
>>
>>  #endif
>> --
>> 1.7.9.5
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/



--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/