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!
