| 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
| ||
|
Message-ID: <CAFnufp0fkdLnYRdcRqE9RzB4eDbbmpfRUqd96Bs1EoFTz2QpVA@mail.gmail.com>
Date: Mon, 9 Nov 2020 16:03:33 +0100
From: Matteo Croce <mcroce@...ux.microsoft.com>
To: Petr Mladek <pmladek@...e.com>
Cc: linux-kernel@...r.kernel.org, Mike Rapoport <rppt@...nel.org>,
Guenter Roeck <linux@...ck-us.net>,
Arnd Bergmann <arnd@...db.de>,
Pavel Tatashin <pasha.tatashin@...een.com>,
Kees Cook <keescook@...omium.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Tyler Hicks <tyhicks@...ux.microsoft.com>
Subject: Re: [PATCH v2] reboot: allow to specify reboot mode via sysfs
On Mon, Nov 9, 2020 at 3:16 PM Petr Mladek <pmladek@...e.com> wrote:
>
> On Fri 2020-11-06 21:07:04, Matteo Croce wrote:
> > From: Matteo Croce <mcroce@...rosoft.com>
> >
> > The kernel cmdline reboot= option offers some sort of control
> > on how the reboot is issued.
> > Add handles in sysfs to allow setting these reboot options, so they
> > can be changed when the system is booted, other than at boot time.
> >
> > The handlers are under <sysfs>/kernel/reboot, can be read to
> > get the current configuration and written to alter it.
> >
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-kernel-reboot
> > @@ -0,0 +1,26 @@
> > +What: /sys/kernel/reboot
> > +Date: October 2020
> > +KernelVersion: 5.11
> > +Contact: Matteo Croce <mcroce@...rosoft.com>
> > +Description: Interface to set the kernel reboot mode, similarly to
> > + what can be done via the reboot= cmdline option.
> > + (see Documentation/admin-guide/kernel-parameters.txt)
> > +
> > +What: /sys/kernel/reboot/mode
> > +What: /sys/kernel/reboot/type
> > +What: /sys/kernel/reboot/cpu
> > +What: /sys/kernel/reboot/force
>
> I do not see any file where it is accumulated this way.
> It seems that each path is always described separately.
>
I've found it in Documentation/ABI/testing/sysfs-kernel-mm-ksm
> I am not sure if it is really needed. But it might be needed
> when processing the API documentation.
>
> Please, split it.
>
Ok
>
> > +
> > +Date: October 2020
> > +Contact: Matteo Croce <mcroce@...rosoft.com>
> > +Description: Tune reboot parameters.
> > +
> > + mode: Reboot mode. Valid values are:
> > + cold warm hard soft gpio
> > +
> > + type: Reboot type. Valid values are:
> > + bios acpi kbd triple efi pci
> > +
> > + cpu: CPU number to use to reboot.
> > +
> > + force: Force an immediate reboot.
> > diff --git a/kernel/reboot.c b/kernel/reboot.c
> > index e7b78d5ae1ab..b9e607517ae3 100644
> > --- a/kernel/reboot.c
> > +++ b/kernel/reboot.c
> > @@ -594,3 +594,196 @@ static int __init reboot_setup(char *str)
> > return 1;
> > }
> > __setup("reboot=", reboot_setup);
> > +
> > +#ifdef CONFIG_SYSFS
> > +
> > +#define STARTS_WITH(s, sc) (!strncmp(s, sc, sizeof(sc)-1))
> > +
> > +static ssize_t mode_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > + const char *val;
> > +
> > + switch (reboot_mode) {
> > + case REBOOT_COLD:
> > + val = "cold\n";
>
> Using "\n" everywhere is weird. Also the same strings are
> repeated in the next functions.
>
> I suggest to define them only once, e.g.
>
> #define REBOOT_COLD_STR "cold"
> #define REBOOT_WARM_STR "warm"
>
> and use here:
>
> case REBOOT_COLD:
> val = REBOOT_COLD_STR;
>
> and then at the end
>
> return sprintf(buf, "%s\n", val);
>
>
Ok.
> > + break;
> > + case REBOOT_WARM:
> > + val = "warm\n";
> > + break;
> > + case REBOOT_HARD:
> > + val = "hard\n";
> > + break;
> > + case REBOOT_SOFT:
> > + val = "soft\n";
> > + break;
> > + case REBOOT_GPIO:
> > + val = "gpio\n";
> > + break;
> > + default:
> > + val = "undefined\n";
> > + }
> > +
> > + return strscpy(buf, val, 10);
>
> "undefined\n" needs 11 bytes to store also the trailing '\0'.
> Anyway, the buffer should be big enough for all variants.
>
>
Oops, missed it.
> > +}
> > +static ssize_t mode_store(struct kobject *kobj, struct kobj_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + if (!capable(CAP_SYS_BOOT))
> > + return -EPERM;
> > +
> > + if (STARTS_WITH(buf, "cold"))
> > + reboot_mode = REBOOT_COLD;
>
> I would prefer to open code this and use strlen(). It will be obvious
> what the code does immediately. And I am sure that compilators
> will optimize out the strlen().
>
>
> if (strncmp(buf, REBOOT_COLD_STR, strlen(REBOOT_COLD_STR)) == 0)
> reboot_mode = REBOOT_COLD;
> else if (strncmp(buf, REBOOT_WARM_STR, strlen(REBOOT_WARM_STR) == 0)
> reboot_mode = REBOOT_WARM;
> ...
>
Yes, since they are constant.
>
>
> > + else if (STARTS_WITH(buf, "warm"))
> > + reboot_mode = REBOOT_WARM;
> > + else if (STARTS_WITH(buf, "hard"))
> > + reboot_mode = REBOOT_HARD;
> > + else if (STARTS_WITH(buf, "soft"))
> > + reboot_mode = REBOOT_SOFT;
> > + else if (STARTS_WITH(buf, "gpio"))
> > + reboot_mode = REBOOT_GPIO;
> > + else
> > + return -EINVAL;
> > +
> > + return count;
> > +}
> > +static struct kobj_attribute reboot_mode_attr = __ATTR_RW(mode);
> > +
> > +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > + const char *val;
> > +
> > + switch (reboot_type) {
> > + case BOOT_TRIPLE:
> > + val = "triple\n";
>
> Same here
>
> var = BOOT_TRIPLE_STR;
>
> > + break;
> > + case BOOT_KBD:
> > + val = "kbd\n";
> > + break;
> > + case BOOT_BIOS:
> > + val = "bios\n";
> > + break;
> > + case BOOT_ACPI:
> > + val = "acpi\n";
> > + break;
> > + case BOOT_EFI:
> > + val = "efi\n";
> > + break;
> > + case BOOT_CF9_FORCE:
> > + val = "cf9_force\n";
> > + break;
> > + case BOOT_CF9_SAFE:
> > + val = "cf9_safe\n";
> > + break;
> > + default:
> > + val = "undefined\n";
> > + }
> > +
> > + return strscpy(buf, val, 10);
> > +}
> > +static ssize_t type_store(struct kobject *kobj, struct kobj_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + if (!capable(CAP_SYS_BOOT))
> > + return -EPERM;
> > +
> > + if (STARTS_WITH(buf, "triple"))
> > + reboot_type = BOOT_TRIPLE;
>
> and here:
>
> if (strncmp(buf, REBOOT_TRIPLE_STR, strlen(REBOOT_TRIPLE_STR)) == 0)
> reboot_type = REBOOT_TRIPLE;
>
>
> > + else if (STARTS_WITH(buf, "kbd"))
> > + reboot_type = BOOT_KBD;
> > + else if (STARTS_WITH(buf, "bios"))
> > + reboot_type = BOOT_BIOS;
> > + else if (STARTS_WITH(buf, "acpi"))
> > + reboot_type = BOOT_ACPI;
> > + else if (STARTS_WITH(buf, "efi"))
> > + reboot_type = BOOT_EFI;
> > + else if (STARTS_WITH(buf, "cf9_force"))
> > + reboot_type = BOOT_CF9_FORCE;
> > + else if (STARTS_WITH(buf, "cf9_safe"))
> > + reboot_type = BOOT_CF9_SAFE;
> > + else
> > + return -EINVAL;
> > +
> > + return count;
> > +}
> > +static struct kobj_attribute reboot_type_attr = __ATTR_RW(type);
> > +
> > +static ssize_t force_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > + return sprintf(buf, "%d\n", reboot_force);
> > +}
> > +static ssize_t force_store(struct kobject *kobj, struct kobj_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + if (!capable(CAP_SYS_BOOT))
> > + return -EPERM;
> > +
> > + if (buf[0] != '0' && buf[0] != '1')
> > + return -EINVAL;
>
> Please use kstrtobool() that supports also other boolean values,
> for example, 'Y', 'n'.
>
Nice, will do.
> > + rc = kstrtouint(buf, 0, &cpunum);
> > +
> > + reboot_force = buf[0] - '0';
> > +
> > + return count;
> > +}
>
> > +static int __init reboot_ksysfs_init(void)
> > +{
> > + struct kobject *reboot_kobj;
> > + int ret;
> > +
> > + reboot_kobj = kobject_create_and_add("reboot", kernel_kobj);
> > + if (!reboot_kobj)
> > + return -ENOMEM;
> > +
> > + ret = sysfs_create_group(reboot_kobj, &reboot_attr_group);
> > + if (ret) {
> > + kobject_put(reboot_kobj);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +core_initcall(reboot_ksysfs_init);
>
> There is no need to create the sysfs interface this early. In fact, it
> might even break because the parent "kernel" node is defined
> as core_initcall() as well. The order is not defined in this case.
>
> I would do it as sybsys_initcall() like or even late_initcall().
>
I'll postpone it to late_initcall().
Thanks,
--
per aspera ad upstream
Powered by blists - more mailing lists