efi only if directory exists.¶

Labels: enhancement, cleanup, fixed / solved / done, minor bug

gozora opened issue at 2018-11-21 17:31:¶

Pull Request Details:¶

Type: Enhancement
Impact: Low
Reference to related issue (URL): https://github.com/rear/rear/issues/1972
How was this pull request tested?
By running rear recover on Arch Linux.
Brief description of the changes in this pull request:
This patch avoids rear recover to end with error if /boot/efi directory does not exist in late recover stage (archive is already restored), and instead adds /boot/efi to SElinux relabeling ignore list, only if /boot/efi directory exists.

gozora commented at 2018-11-21 17:58:¶

After thinking about this PR for a while, I've decided for less invasive and IMHO more correct way ...

V.

jsmeix commented at 2018-11-22 08:27:¶

Argh!

@gozora
thank you to call my attention to that script.

I never had a look at that script before because I do not use SELinux.
In general I do not like unconditioned special case handling.

The following issues exist since the beginning in that script:

With that script every user who has UEFI (and /boot/efi) gets a new file
/etc/selinux/fixfiles_exclude_dirs created during "rear recover".

What about users who do not use SELinux?
They get a useless /etc/selinux/fixfiles_exclude_dirs created.

What about users who have already a /etc/selinux/fixfiles_exclude_dirs file?
They get their /etc/selinux/fixfiles_exclude_dirs overwritten.

This means two more conditions need to be checked:

(a)
Do nothing when SELinux is not used.

(b)
Do not silently overwrite user's files.
An existing file is user data that was restored from the his backup and
user data is usually sacrosanct unless the user confirmed otherwise
or at least ReaR has to save user's files before overwriting them, cf.
https://github.com/rear/rear/issues/1854#issuecomment-403013334 and
https://github.com/rear/rear/commit/632bc2dee4d5b82cfe91416c81254f0bc9919a70

@gozora
because you are already working on it I would very much appreciate it
if you could also clean up that script in general.

gozora commented at 2018-11-22 09:37:¶

Hello @jsmeix

My knowledge of SELinux is limited to disabling of this feature ;-)

(a)
Do nothing when SELinux is not used.

I'm not sure if we can detect SELinux usage and mode of operation reliably across all distributions...

Do not silently overwrite user's files.

Fully agree, Maybe using:

-  cat > $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs <<EOF
+  cat >> $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs <<EOF
...

Would be better ...

V.

jsmeix commented at 2018-11-22 10:14:¶

@gdha
I assigned you too here because I assume you could help
with the SELinux related questions here.

But there is no need to do the general clean up right now here.
We could also do it later in a separated step as time permits.

jsmeix commented at 2018-11-22 10:21:¶

@gozora
I also know nothing at all about SELinux setup.

But even without any SELinux knowledge only by plain looking at the code
I assume at the beginning an additional condition like

# Skip if there is no etc/selinux/ directory (e.g. when SELinux is not used)
# and if there is no etc/selinux/ directory creating a fixfiles_exclude_dirs file
# therein cannot work anyway:
test -d $TARGET_FS_ROOT/etc/selinux || return 0

is needed to make the subsequent code work fail-safe.

jsmeix commented at 2018-11-22 10:33:¶

A proposal how to clean it up and make it work more straightforward:

# For UEFI we should avoid SElinux relabeling vfat filesystem /boot/efi

# Skip if UEFI is not used:
is_true $USING_UEFI_BOOTLOADER || return 0

# Skip if there is no etc/selinux/ directory (e.g. when SELinux is not used)
# and if there is no etc/selinux/ directory creating a fixfiles_exclude_dirs
# file therein cannot work anyway:
test -d $TARGET_FS_ROOT/etc/selinux || return 0

# Skip if there is no boot/efi directory:
test -d $TARGET_FS_ROOT/boot/efi || return 0

# Do not overwrite an existing etc/selinux/fixfiles_exclude_dirs file
# An existing file is user data that was restored from his backup and
# user data is sacrosanct unless the user had confirmed otherwise:
if test -s $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs ; then
    Log "Not overwriting the existing etc/selinux/fixfiles_exclude_dirs file"
    return 0
fi

# Create etc/selinux/fixfiles_exclude_dirs file from scratch:
cat > $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs <<EOF
/boot/efi
/boot/efi(/.*)?
EOF

By the way:
What if /boot/efi is not a mounted vfat filesystem?
Can this happen with UEFI?
I think this cannot happen with UEFI on Linux because
https://en.wikipedia.org/wiki/EFI_system_partition
reads

The EFI system partition needs to be formatted
with a file system whose specification is based
on the FAT file system and maintained as part
of the UEFI specification; therefore, the file system
specification is independent from the original
FAT specification.

But what if the ESP is not mounted at all (can this happen on Linux?)?
or if the ESP is mounted but not at /boot/efi (e.g. on Arch Linux)?
https://en.wikipedia.org/wiki/EFI_system_partition
reads

The mount point for the EFI system partition
is usually /boot/efi, where its content is
accessible after Linux is booted

The "usually" indicates we cannot rely on ESP mounted at /boot/efi
and I don't know if the ESP content needs to be accessible after
Linux is booted in any case.

gozora commented at 2018-11-22 10:47:¶

@jsmeix
Nice one!
I'll update PR later today.
Thanks for your help!

V.

jsmeix commented at 2018-11-22 13:55:¶

Using code to "Determine where the EFI System Partition (ESP) is mounted" from
https://github.com/rear/rear/commit/14bfcd40d8d78ba6c45db11cbd31b2f83d0fb6c2

# Determine where the EFI System Partition (ESP) is mounted in the currently running recovery system:
esp_mountpoint=$( df -P "$TARGET_FS_ROOT/$UEFI_BOOTLOADER" | tail -1 | awk '{print $6}' )
# Use TARGET_FS_ROOT/boot/efi as fallback ESP mountpoint:
test "$esp_mountpoint" || esp_mountpoint="$TARGET_FS_ROOT/boot/efi"

should sufficiently mitigate possible issues where ESP is mounted
also in this case here.

Perhaps also the test in
https://github.com/rear/rear/commit/14bfcd40d8d78ba6c45db11cbd31b2f83d0fb6c2

# UEFI_BOOTLOADER empty or not a regular file means using BIOS cf. rescue/default/850_save_sysfs_uefi_vars.sh
# Double quotes are mandatory here because 'test -f' without any (possibly empty) argument results true:
test -f "$UEFI_BOOTLOADER" || return 0

could be useful in this case here?

If I am right, those duplicated testing code snippets in several scripts
might be later combined and moved into one general function...

jsmeix commented at 2018-11-22 14:28:¶

Enhanced proposal how to clean it up
as in https://github.com/rear/rear/pull/1977#issuecomment-440986677
plus https://github.com/rear/rear/pull/1977#issuecomment-441037816

# For UEFI we should avoid SELinux relabeling the
# vfat filesystem of the EFI System Partition (ESP)
# which is usually mounted at boot/efi

# Skip if there is no etc/selinux/ directory (e.g. when SELinux is not used)
# and if there is no etc/selinux/ directory creating a fixfiles_exclude_dirs
# file therein (see the code at the end) cannot work anyway:
test -d $TARGET_FS_ROOT/etc/selinux || return 0

# The following four code parts are same also in
# finalize/Linux-i386/630_run_efibootmgr.sh

# USING_UEFI_BOOTLOADER empty or not true means using BIOS
is_true $USING_UEFI_BOOTLOADER || return 0

# UEFI_BOOTLOADER empty or not a regular file means using BIOS cf. rescue/default/850_save_sysfs_uefi_vars.sh
# Double quotes are mandatory here because 'test -f' without any (possibly empty) argument results true:
test -f "$UEFI_BOOTLOADER" || return 0

# Determine where the EFI System Partition (ESP) is mounted in the currently running recovery system:
esp_mountpoint=$( df -P "$TARGET_FS_ROOT/$UEFI_BOOTLOADER" | tail -1 | awk '{print $6}' )
# Use TARGET_FS_ROOT/boot/efi as fallback ESP mountpoint:
test "$esp_mountpoint" || esp_mountpoint="$TARGET_FS_ROOT/boot/efi"

# Skip if there is no esp_mountpoint directory (e.g. the fallback ESP mountpoint may not exist).
# Double quotes are mandatory here because 'test -d' without any (possibly empty) argument results true:
test -d "$esp_mountpoint" || return 0

# Do not overwrite an existing etc/selinux/fixfiles_exclude_dirs file
# An existing file is user data that was restored from his backup and
# user data is sacrosanct unless the user had confirmed otherwise:
if test -s $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs ; then
    Log "Not overwriting the existing etc/selinux/fixfiles_exclude_dirs file"
    return 0
fi

# The ESP mountpoint directory values in fixfiles_exclude_dirs
# must match what there will be on the recreated target system
# i.e. esp_mountpoint without TARGET_FS_ROOT prefix:
target_esp_mountpoint=${esp_mountpoint#$TARGET_FS_ROOT}

# Create etc/selinux/fixfiles_exclude_dirs file from scratch:
cat > $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs <<EOF
$target_esp_mountpoint
$target_esp_mountpoint(/.*)?
EOF

FYI:
I added the "Skip if there is no esp_mountpoint directory"
also to finalize/Linux-i386/630_run_efibootmgr.sh via
https://github.com/rear/rear/commit/cb6fe3be719c1361b22d0484cd940c681752a973

gozora commented at 2018-11-22 17:37:¶

@jsmeix
Just for a record, I guess you've meant:

-  cat < $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs <<EOF
+  cat > $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs <<EOF
$target_esp_mountpoint
$target_esp_mountpoint(/.*)?
EOF

V.

jsmeix commented at 2018-11-23 08:06:¶

@gozora
of course it must be
cat > $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs <<EOF
(I fixed it in my comments above).

For the (not so) fun of it:
The reason for the typo was that I use <pre> ... </pre>
for preformatted text blocks but within <pre> ... </pre>
one cannot use < or > literally because they are reserverd HTML charaters
so that to display < or > one must use their HTML entities < and >
and I had accidentally used < for > instead of > but now I learned
that I can also use three backticks ``` and even how to display backticks
as backticks by having them in backticks plus leading and trailing spaces,
it's just so easy to simply write Markdown that even shows plain ASCII text
(braindead modern world)...

gdha commented at 2018-11-26 09:14:¶

@schlomo are you fine with this PR?

schlomo commented at 2018-11-26 10:44:¶

@gozora @gdha I cannot form an opinion as I am too far away from this problem.

Just wondering, if the fixfiles_exclude_dirs file exists and does not contain the ESP mount point, will the recovered system still work? Why not simply inject/append the ESP there?

jsmeix commented at 2018-11-26 11:32:¶

Because this is only about to avoid SElinux relabeling the vfat filesystem of the ESP
my assumption was that it is "the right thing" to skip the actual action in that script
(which is creating etc/selinux/fixfiles_exclude_dirs file from scratch)
when any of the needed conditions is not met.

gdha commented at 2018-11-26 14:03:¶

@gozora You can merge this PR from my point of view.

rmetrich commented at 2019-04-10 07:02:¶

@gozora Sorry for the late wakeup, didn't see this until now, but adding /boot/efi to exclusion list is not needed at all. Internally, SELinux's fixfiles checks if the file system supports security labels, and VFAT doesn't, so it never tries to relabel /boot/efi.

gozora commented at 2019-04-10 07:15:¶

Hello @rmetrich,

Reading https://github.com/rear/rear/issues/1972, this PR was mostly about correcting

if ! test -d "$TARGET_FS_ROOT/boot/efi" ; then
    Error "Could not find directory $TARGET_FS_ROOT/boot/efi"
fi

whew 510_selinux_fixfiles_exclude_dirs.sh runs.

/boot/efi was part of $TARGET_FS_ROOT/etc/selinux/fixfiles_exclude_dirs even before this PR ...

But honestly I did not spent much time hacking SELinux, so if you think that /boot/efi is not needed in this list, feel free to update the code ;-).

V.

[Export of Github issue for rear/rear.]

#1977 PR merged: Ignore relabeling of /boot/efi only if directory exists.¶