linux-kernel - Re: [PATCH v7 2/5] x86: Add cmdline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Nov 2022 11:59:49 +0300
From:   Evgeniy Baskov <baskov@...ras.ru>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Dave Hansen <dave.hansen@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        Alexey Khoroshilov <khoroshilov@...ras.ru>
Subject: Re: [PATCH v7 2/5] x86: Add cmdline_prepare() helper

On 2022-11-10 01:54, Borislav Petkov wrote:
> On Tue, Oct 04, 2022 at 03:48:21PM +0300, Evgeniy Baskov wrote:
>> +static inline void cmdline_prepare(char *command_line,
>> +				   const char *boot_command_line)
>> +{
>> +	if (!IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
>> +		strscpy(command_line, boot_command_line, COMMAND_LINE_SIZE);
>> +	} else if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
>> +		/* Append boot loader cmdline to builtin one. */
>> +		strlcat(command_line, " ", COMMAND_LINE_SIZE);
>> +		strlcat(command_line, boot_command_line, COMMAND_LINE_SIZE);
>> +	}
> 
> Why can't we make this plain and simple and understandable at a quick
> glance instead of putting parts of the string in some variable and then
> still doing copying outside of the function?
> 
> IOW, simply put everything in a single function:
> 
> static inline void cmdline_prepare(char *dst,
>                                    const char *builtin_cmdline,
>                                    const char *boot_command_line)
> {
>         /* Depends on CONFIG_CMDLINE_BOOL, overwrite with builtin 
> cmdline */
>         if (IS_ENABLED(CONFIG_CMDLINE_OVERRIDE))
>                 strscpy(dst, builtin_cmdline, COMMAND_LINE_SIZE);
>         else if (IS_ENABLED(CONFIG_CMDLINE_BOOL)) {
>                 if (builtin_cmdline[0]) {
>                         /* append boot loader cmdline to builtin */
>                         srtlcat(dst, builtin_cmdline, 
> COMMAND_LINE_SIZE)
>                         strlcat(dst, " ", COMMAND_LINE_SIZE);
>                         strlcat(dst, boot_command_line, 
> COMMAND_LINE_SIZE);
>                 } else {
>                         strscpy(dst, boot_command_line, 
> COMMAND_LINE_SIZE);
>                 }
>         }
> }
> 
> which is then called like this:
> 
> 	cmdline_prepare(command_line, builtin_cmdline, boot_command_line);
> 
> and when it returns command_line is ready. And before it, command_line
> is the empty string:
> 
> static char __initdata command_line[COMMAND_LINE_SIZE];
> 
> And all the complexity is in one single function and that single
> function does each case in an obvious manner and you don't have to go
> look outside of the function body to understand what it does.
> 
> Hmmm?

Well, my version takes one less copy... But clarity is more important
in the kernel, I guess. I'll send updated version soon.

Thanks,
Evgeniy Baskov