Re: [RFC v3 1/2] kernel/sysctl: support setting sysctl parameters from kernel command line

[Vlastimil Babka <vbabka@suse.cz>]

On 3/26/20 9:24 PM, Kees Cook wrote:
> On Thu, Mar 26, 2020 at 07:16:05PM +0100, Vlastimil Babka wrote:
>> Since the major change, I'm sending another RFC. If this approach is ok, then
>> it probably needs just some tweaks to the various error prints, and then
>> converting the rest of existing on-off aliases (if I come up with an idea how
>> to find them all). Thanks for all the feedback so far.
> 
> Yeah, I think you can drop "RFC" from this in the next version -- you're
> well into getting this finalized IMO.

Thanks!

>>
>>  .../admin-guide/kernel-parameters.txt         |  9 ++
>>  fs/proc/proc_sysctl.c                         | 90 +++++++++++++++++++
>>  include/linux/sysctl.h                        |  4 +
>>  init/main.c                                   |  2 +
>>  4 files changed, 105 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index c07815d230bc..0c7e032e7c2e 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4793,6 +4793,15 @@
>>  
>>  	switches=	[HW,M68k]
>>  
>> +	sysctl.*=	[KNL]
>> +			Set a sysctl parameter, right before loading the init
>> +			process, as if the value was written to the respective
>> +			/proc/sys/... file. Unrecognized parameters and invalid
>> +			values are reported in the kernel log. Sysctls
>> +			registered later by a loaded module cannot be set this
>> +			way.
> 
> Maybe add: "Both '.' and '/' are recognized as separators."

OK

>> +
>> +/* Set sysctl value passed on kernel command line. */
>> +static int process_sysctl_arg(char *param, char *val,
>> +			       const char *unused, void *arg)
>> +{
>> +	char *path;
>> +	struct file_system_type *proc_fs_type;
>> +	struct file *file;
>> +	int len;
>> +	int err;
>> +	loff_t pos = 0;
>> +	ssize_t wret;
>> +
>> +	if (strncmp(param, "sysctl", sizeof("sysctl") - 1))
>> +		return 0;
>> +
>> +	param += sizeof("sysctl") - 1;
>> +
>> +	if (param[0] != '/' && param[0] != '.')
>> +		return 0;
>> +
>> +	param++;
>> +
>> +	if (!proc_mnt) {
>> +		proc_fs_type = get_fs_type("proc");
>> +		if (!proc_fs_type) {
>> +			pr_err("Failed to mount procfs to set sysctl from command line");
>> +			return 0;
>> +		}
>> +		proc_mnt = kern_mount(proc_fs_type);
>> +		put_filesystem(proc_fs_type);
>> +		if (IS_ERR(proc_mnt)) {
>> +			pr_err("Failed to mount procfs to set sysctl from command line");
>> +			proc_mnt = NULL;
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	len = 4 + strlen(param) + 1;
>> +	path = kmalloc(len, GFP_KERNEL);
>> +	if (!path)
>> +		panic("%s: Failed to allocate %d bytes t\n", __func__, len);
>> +
>> +	strcpy(path, "sys/");
>> +	strcat(path, param);
>> +	strreplace(path, '.', '/');
> 
> You can do the replacement against the param directly, and also avoid
> all the open-coded string manipulations:
> 
> 	strreplace(param, '.', '/');

I didn't want to modify param for the sake of error prints, but perhaps
the replacements won't confuse system admin too much?

> 	path = kasprintf(GFP_KERNEL, "sys/%s", param);

Ah yea that's nicer.

>> +
>> +	file = file_open_root(proc_mnt->mnt_root, proc_mnt, path, O_WRONLY, 0);
>> +	if (IS_ERR(file)) {
>> +		err = PTR_ERR(file);
>> +		pr_err("Error %d opening proc file %s to set sysctl parameter '%s=%s'",
>> +			err, path, param, val);
>> +		goto out;
>> +	}
>> +	len = strlen(val);
>> +	wret = kernel_write(file, val, len, &pos);
>> +	if (wret < 0) {
>> +		err = wret;
>> +		pr_err("Error %d writing to proc file %s to set sysctl parameter '%s=%s'",
>> +			err, path, param, val);
>> +	} else if (wret != len) {
>> +		pr_err("Wrote only %ld bytes of %d  writing to proc file %s to set sysctl parameter '%s=%s'",
>> +			wret, len, path, param, val);
>> +	}
>> +
>> +	filp_close(file, NULL);
> 
> Please check the return value of filp_close() and treat that as an error
> for this function too.

Well I could print it, but not much else? The unmount will probably fail
in that case?

>> +out:
>> +	kfree(path);
>> +	return 0;
>> +}
>> +
>> +void do_sysctl_args(void)
>> +{
>> +	char *command_line;
>> +
>> +	command_line = kstrdup(saved_command_line, GFP_KERNEL);
>> +	if (!command_line)
>> +		panic("%s: Failed to allocate copy of command line\n", __func__);
>> +
>> +	parse_args("Setting sysctl args", command_line,
>> +		   NULL, 0, -1, -1, NULL, process_sysctl_arg);
>> +
>> +	if (proc_mnt)
>> +		kern_unmount(proc_mnt);
> 
> I don't recommend sharing allocation lifetimes between two functions
> (process_sysctl_arg() allocs proc_mnt, and do_sysctl_args() frees it).
> And since you have a scoped lifetime, why allocate it or have it as a
> global at all? It can be stack-allocated and passed to the handler:

So the point was that the mount is only done when an applicable sysctl
parameter is found. On majority of systems there won't be any, at least
for initial X years :)

> void do_sysctl_args(void)
> {
> 	struct file_system_type *proc_fs_type;
> 	struct vfsmount *proc_mnt;
> 	char *command_line;
> 
> 	proc_fs_type = get_fs_type("proc");
> 	if (!proc_fs_type) {
> 		pr_err("Failed to mount procfs to set sysctl from command line");
> 		return;
> 	}
> 	proc_mnt = kern_mount(proc_fs_type);
> 	put_filesystem(proc_fs_type);
> 	if (IS_ERR(proc_mnt)) {
> 		pr_err("Failed to mount procfs to set sysctl from command line");
> 		return;
> 	}
> 
> 	command_line = kstrdup(saved_command_line, GFP_KERNEL);
> 	if (!command_line)
> 		panic("%s: Failed to allocate copy of command line\n",
> 			__func__);
> 
> 	parse_args("Setting sysctl args", command_line,
> 		   NULL, 0, -1, -1, proc_mnt, process_sysctl_arg);
> 
> 	kfree(command_line);
> 	kern_unmount(proc_mnt);
> }
> 
> And then pull the mount from (the hilariously overloaded name) "arg":

But I guess the "mount on first applicable argument" approach would work
with this scheme as well:

struct vfsmount *proc_mnt = NULL;
parse_args(..., &proc_mnt, ...)

Thanks!