[systemd-devel] [PATCH v3] Add reboot to EFI support

[Lennart Poettering]

On Fri, 03.04.15 12:40, Jan Janssen (medhefgo at web.de) wrote:

> Changes in v3:
>  - call the feature reboot to firmware everywhere
>  - make the login interface a property and methods to change it and
>    don't couple it with a reboot action
>  - changed/added policykit action defaulting to auth_admin_keep. Please
>    change this if something else is desired.
> 
> The policy kit behavior feels weird to me: if I call bus_verify_polkit()
> with interactive = false, I still get a password prompt for
> CanRebootToFirmware.

Hmm, I figure this is because of the bus message header flag that asks
for interactivity for all calls now. i.e. in systemctl we call
sd_bus_set_allow_interactive_authorization() once, which makes all
calls interactive. This is a general bug in bus-util.c, which I need
to fix. 

Ignore this for now, I will fix this, I added this to the TODO list.

> Is this a bug or am I doing something wrong here? Do I need to make a separate
> get policy (with no auth_admin_keep) for this to work?

>  static int method_inhibit(sd_bus *bus, sd_bus_message *message, void *userdata, sd_bus_error *error) {
>          _cleanup_bus_creds_unref_ sd_bus_creds *creds = NULL;
>          const char *who, *why, *what, *mode;
> @@ -1970,6 +2063,7 @@ const sd_bus_vtable manager_vtable[] = {
>          SD_BUS_PROPERTY("KillOnlyUsers", "as", NULL, offsetof(Manager, kill_only_users), SD_BUS_VTABLE_PROPERTY_CONST),
>          SD_BUS_PROPERTY("KillExcludeUsers", "as", NULL, offsetof(Manager, kill_exclude_users), SD_BUS_VTABLE_PROPERTY_CONST),
>          SD_BUS_PROPERTY("KillUserProcesses", "b", NULL, offsetof(Manager, kill_user_processes), SD_BUS_VTABLE_PROPERTY_CONST),
> +        SD_BUS_PROPERTY("RebootToFirmware", "b", property_get_reboot_to_firmware, 0, SD_BUS_VTABLE_PROPERTY_CONST),
>          SD_BUS_PROPERTY("IdleHint", "b", property_get_idle_hint, 0, SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
>          SD_BUS_PROPERTY("IdleSinceHint", "t", property_get_idle_since_hint, 0, SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
>          SD_BUS_PROPERTY("IdleSinceHintMonotonic", "t", property_get_idle_since_hint, 0, SD_BUS_VTABLE_PROPERTY_EMITS_CHANGE),
> @@ -2023,6 +2117,8 @@ const sd_bus_vtable manager_vtable[] = {
>          SD_BUS_METHOD("CanHibernate", NULL, "s", method_can_hibernate, SD_BUS_VTABLE_UNPRIVILEGED),
>          SD_BUS_METHOD("CanHybridSleep", NULL, "s", method_can_hybrid_sleep, SD_BUS_VTABLE_UNPRIVILEGED),
>          SD_BUS_METHOD("Inhibit", "ssss", "h", method_inhibit, SD_BUS_VTABLE_UNPRIVILEGED),
> +        SD_BUS_METHOD("SetRebootToFirmware", "bb", NULL, method_set_reboot_to_firmware, SD_BUS_VTABLE_UNPRIVILEGED),
> +        SD_BUS_METHOD("CanRebootToFirmware", NULL, "s",
> method_can_reboot_to_firmware, SD_BUS_VTABLE_UNPRIVILEGED),

Looks OK, but I think here too it should say FirmwareSetup rather than
Firmware:

        RebootToFirmwareSetup
        SetRebootToFirmwareSetup
        CanRebootToFirmwareSetup

Similar for the PK action name...

>  
> +bool is_efi_reboot_to_fw_supported(void) {

So far all calls exported by efivars.[ch] used "efi_" is prefix, and I
think this call should too.

Also, the acronym "fw" we already use in the firewall context. I'd
prefer if we would call this efi_reboot_to_firmware_supported() or
so. Similar for the other exported calls.


> +        int r;
> +        uint64_t b;
> +
> +        r = get_os_indications_supported(&b);
> +        return r >= 0;
> +}

I figure assigning this to r first is unnecessary? 

Shouldn't get_os_indications_supported() already act boolean-like,
i.e. return negative on error, 0 when the os indications feature is
not supported and > 0 if it is supported?

Maybe get_os_indications_supported should actually be the exported
function, but optionally take NULL as parameter for those callers that
are not interested in the actual bitfield? With that change
is_efi_reboot_to_fw_supported() and get_os_indications_supported() are
pretty much identical no, and could just be one function?

> +int efi_get_reboot_to_fw(void) {
> +        int r;
> +        uint64_t b;
> +
> +        r = get_os_indications(&b);
> +        return r < 0 ? r : !!(b & EFI_OS_INDICATIONS_BOOT_TO_FW_UI);
> +}

I think this would be easier to read without the ternary operator, and
a simple if.

> +
> +int efi_set_reboot_to_fw(bool value) {
> +        int r;
> +        uint64_t b;
> +
> +        r = get_os_indications(&b);
> +        if (r < 0)
> +                return r;
> +
> +        if (value)
> +                b |= EFI_OS_INDICATIONS_BOOT_TO_FW_UI;
> +        else
> +                b &= ~EFI_OS_INDICATIONS_BOOT_TO_FW_UI;

I think it would be wise to suppress updating the variable if the bit
is already set correctly. After all writes to this memory are
problematic and should be kept at a minimum?

Lennart

-- 
Lennart Poettering, Red Hat