fix: mount /boot/efi with restrictive fmask/dmask#309
fix: mount /boot/efi with restrictive fmask/dmask#309gagath merged 1 commit intoqualcomm-linux:mainfrom
Conversation
|
This warning is triggered by systemd as part of its view on how filesystems should be used; could you confirm how the ESP gets automounted in the systemd world (with which flags)? I suspect this warning also affects Debian users using systemd-boot, I guess that's a non-standard option. How do we get this in Debian? |
If it helps, I did notice the same warning in the journal at boot time. So if that's confirmed to disappear with this fix, then maybe we don't need to worry about it at image build time. |
Test jobs for commit cdfc90f |
I think this is dependent of the Debian Installer that will create the
Agreed. I will do a test-boot with the image generated with this fix and see what happens. If the warning goes away, then it is a win already. |
|
With this change, after booting the image in QEMU the $ ls -lh /boot
...
drwx------ 6 root root 4.0K Jan 1 1970 efi
...
$ ls /boot/efi/
ls: cannot open directory '/boot/efi/': Permission deniedAnd the warning messages regarding the insecure Marking as ready for review. Note that the commit does not |
lool
left a comment
lool
left a comment
There was a problem hiding this comment.
If that's easy, could you share how the fstab will look like in the end? Since we're setting mount options, we might as well them them fully including nodev, nosuid, noexec – seems fitting for ESP.
There's a typo in your commit message: randon-seed
| - mountpoint: /boot/efi/ | ||
| partition: esp | ||
| options: | ||
| - fmask=0077,dmask=0077 |
There was a problem hiding this comment.
I guess umask would have worked for this, but since you set both, I think fmask=0177 is a bit more strict; perhaps defaults,nodev,nosuid,noexec too
I'm taking this from e.g. https://wiki.archlinux.org/title/Fstab but I guess d-i would be a more authoritative source if it sets any mask.
There was a problem hiding this comment.
I tried to find how systemd sets the flags, but it doesn't seem to https://github.com/systemd/systemd/blob/main/src/gpt-auto-generator/gpt-auto-generator.c
There was a problem hiding this comment.
but it doesn't seem to
That's puzzling, IIUC. I'd expect systemd to do the right thing in mounting the ESP when used with systemd-boot by default.
There was a problem hiding this comment.
Hm, why isn't systemd mounting this automatically?
It works fine on meta-qcom, it mounts in /boot when the directory is empty or at /efi as a fallback.
/dev/mmcblk0p72 on /efi type vfat (rw,nosuid,nodev,noexec,relatime,nosymfollow,fmask=0177,dmask=0077,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
There was a problem hiding this comment.
root@uno-q:/boot# bootctl
System:
Firmware: UEFI 2.110 (Das U-Boot 8230.256)
Firmware Arch: aa64
Secure Boot: disabled (setup)
TPM2 Support: no
Measured UKI: no
Boot into FW: not supported
Current Boot Loader:
Product: systemd-boot 258.1
Features: ✓ Boot counting
✓ Menu timeout control
✓ One-shot menu timeout control
✓ Default entry control
✓ One-shot entry control
✓ Support for XBOOTLDR partition
✓ Support for passing random seed to OS
✓ Load drop-in drivers
✓ Support Type #1 sort-key field
✓ Support @saved pseudo-entry
✓ Support Type #1 devicetree field
✓ Enroll SecureBoot keys
✓ Retain SHIM protocols
✓ Menu can be disabled
✓ Multi-Profile UKIs are supported
✓ Loader reports network boot URL
✓ Support Type #1 uki field
✓ Support Type #1 uki-url field
✓ Loader reports TPM2 active PCR banks
Partition: /dev/disk/by-partuuid/beb87cc1-78a0-f7aa-679d-8bbf294205d2
Loader: └─/efi///EFI/BOOT/BOOTAA64.EFI
Current Entry: linux-uno-q.efi
Current Stub:
Product: systemd-stub 258.1
Features: ✓ Stub reports loader partition information
✓ Stub reports stub partition information
✓ Stub reports network boot URL
✓ Picks up credentials from boot partition
✓ Picks up system extension images from boot partition
✓ Picks up configuration extension images from boot partition
✓ Measures kernel+command line+sysexts
✓ Support for passing random seed to OS
✓ Pick up .cmdline from addons
✓ Pick up .cmdline from SMBIOS Type 11
✓ Pick up .dtb from addons
✓ Stub understands profile selector
Partition: /dev/disk/by-partuuid/beb87cc1-78a0-f7aa-679d-8bbf294205d2
Stub: └─//EFI/Linux/linux-uno-q.efi
Random Seed:
System Token: not set
Exists: yes
Available Boot Loaders on ESP:
ESP: /efi (/dev/disk/by-partuuid/beb87cc1-78a0-f7aa-679d-8bbf294205d2)
File: └─/efi//EFI/BOOT/bootaa64.efi (systemd-boot 258.1)
No boot loaders listed in EFI Variables.
Boot Loader Entry Locations:
ESP: /efi (/dev/disk/by-partuuid/beb87cc1-78a0-f7aa-679d-8bbf294205d2, $BOOT)
config: /efi//loader/loader.conf: No such file or directory
token: qcom-distro
Default Boot Loader Entry:
type: Boot Loader Specification Type #2 (UKI, .efi)
title: Qualcomm Linux Reference Distro 2.0
id: linux-uno-q.efi
source: /efi//EFI/Linux/linux-uno-q.efi (on the EFI System Partition)
sort-key: qcom-distro
version: 2.0
linux: /efi//EFI/Linux/linux-uno-q.efi
options: root=PARTLABEL=rootfs rw rootwait console=ttyMSM0,115200 clk_ignore_unused pd_ignore_unused deferred_probe_timeout=30>
There was a problem hiding this comment.
Yeah, part of https://uapi-group.org/specifications/specs/discoverable_partitions_specification/ and https://www.freedesktop.org/software/systemd/man/latest/systemd-gpt-auto-generator.html.
But I guess it only works properly with systemd-boot, needs EFI variable LoaderDevicePartUUID of the 4a67b082-0a4c-41cf-b6c7-440b29bb8c4f vendor UUID.
There was a problem hiding this comment.
I think we can make it compatible with both, but perhaps we should keep /etc/fstab to remain compatible with non-systemd too, for as long as Debian is.
There was a problem hiding this comment.
Yes, I went through the same line of thinking, and also want fstab to support both GRUB and systemd-boot.
I wanted to find the set of flags that systemd uses for ESP by default to follow its practice and have a solid reference point to track, but didn't quite pin it down so far.
There was a problem hiding this comment.
I just pasted above :-)
/dev/mmcblk0p72 on /efi type vfat (rw,nosuid,nodev,noexec,relatime,nosymfollow,fmask=0177,dmask=0077,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
There was a problem hiding this comment.
I just pasted above :-)
/dev/mmcblk0p72 on /efi type vfat (rw,nosuid,nodev,noexec,relatime,nosymfollow,fmask=0177,dmask=0077,codepage=437,iocharset=iso8859-1,shortname=mixed,errors=remount-ro)
Ah yes, saw your comment, I should acknowledged that, thanks for sharing
I wanted to connect with the upstream source code so that we would have an OS-agnostic reference to link to and check over time. Reading the flags from a Yocto boot I thought they'd be mixed with other flags, but I guess if it's automounted, it's only systemd that is setting these.
|
debos seems to hardcode the If we want to reproduce the full fstab line without the "default" option then it seems that we would need to modify debos as well? |
I think we're fine with defaults at the start? fstab(5) says these are set by kernel and filesystem, so should be reasonable, and we can override with the set that systemd uses for ESPs, so fstab will have: It's because these are non-trivial that I'd prefer them to be from some upstream of some kind, so that we're not maintaining something unique in this specific repo. (If you need to propose changes to debos, do propose changes to debos obviously though :)) |
Also, can you add |
|
Below is the content of the generated Test-booted on QEMU, |
gagath
force-pushed
the
mount-boot-efi-fmask
branch
2 times, most recently
from
51f4066 to
d830668
Compare
|
Do we need a test that ensures that the warning journal entry is absent? We could use a qemu test for that. If that slows things down too much, we can think about marking tests and then not running all of them all the time. |
I would happily add a test that checks that the string do not appear. However it seems we have zero tests currently, and implementing them would be a bit out of topic regarding this PR. I think you should create a new issue about this so we can prototype non-regression tests around QEMU in another PR, referencing this fix as an example. My two cents. |
|
We have https://github.com/qualcomm-linux/qcom-deb-images/blob/main/ci/qemu_test.py#L67 which should be a reasonable model for this I think. Maybe asking journalctl would be better, but checking it doesn't come out of serial might be reasonable for now. |
|
Also, to avoid duplicating work, #199 exists but isn't landed (I didn't consider it a priority before as it wasn't holding anything else up). |
Test jobs for commit 51f4066 |
Test jobs for commit d830668 |
|
I added a test. It is dirty because of how we have to login on the serial console, but is better than no tests. We need to figure out this test situation indeed, but I think that for this PR it should be enough. :) |
gagath
force-pushed
the
mount-boot-efi-fmask
branch
from
d830668 to
d75aa4b
Compare
Test jobs for commit d75aa4b |
gagath
force-pushed
the
mount-boot-efi-fmask
branch
from
d75aa4b to
9db3b7c
Compare
|
The test was always succeeding because of the local serial echo. Duplicated the |
Test jobs for commit 9db3b7c |
basak-qcom
left a comment
basak-qcom
left a comment
There was a problem hiding this comment.
Looks good to merge, thanks! I left one inline comment requesting a code comment. Sorry I didn't consider that earlier. Up to you whether you want this or not, and no need for me to review further.
| mountpoints: | ||
| - mountpoint: /boot/efi/ | ||
| partition: esp | ||
| options: |
There was a problem hiding this comment.
I wanted to double check these options and see them discussed in the PR. IIUC, these options are meant to track exactly what systemd does by default when mounting discoverable partitions, right? In that case, could we have a comment here that says that we're tracking that, please, to avoid drift in these options over time without thinking about divergence from systemd behaviour?
There was a problem hiding this comment.
I agree, thanks for being thorough. Added a comment explaining where the options come from as well as referencing both this PR and the issue that initiated it.
systemd-boot creates a /boot/efi/loader/random-seed file that should not be accessible by users. Yet, because the backing filesystem is FAT32, by default everyone can read files when the filesystem is mounted. Add fmask and dmask options to the generated /etc/fstab to deny read access to regular users when /boot is mounted, as well as other common options for the ESP partition. Closes: qualcomm-linux#279 Signed-off-by: Agathe Porte <agathe.porte@oss.qualcomm.com>
gagath
force-pushed
the
mount-boot-efi-fmask
branch
from
9db3b7c to
11e412b
Compare
Test jobs for commit 11e412b |
|
Approved; not in love with the test there, but it is what it is :) I think we need a more reliable channel for the qemu tests; perhaps adb or ssh or some other architecture |
I believe this is what @basak-qcom wants to achieve in #199, to be followed. |
4 participants
systemd-boot creates a /boot/efi/loader/randon-seed file that should not be accessible by users. Yet, because the backing filesystem is FAT32, by default everyone can read files when the filesystem is mounted.
Add fmask and dmask options to the generated /etc/fstab to deny read access to regular users when /boot is mounted.
This is a draft for now as it is an attempt but currently do not remove the warning when generating the image. Maybe it is because debos creates the /etc/fstab file correctly, but mounts it without using the fmask and dmask options regardless, leading to security warnings when generating the image.
Need to investigate if the message disappears when the command is run from a live system, and to maybe create an issue regarding debos itself if the passed options are not respected when building the image.
See #279 for context.