On 10/08/19 12:55, Javier Martinez Canillas wrote: > The driver exposes EFI runtime services to user-space through an IOCTL > interface, calling the EFI services function pointers directly without > using the efivar API. > > Disallow access to the /dev/efi_test character device when the kernel is > locked down to prevent arbitrary user-space to call EFI runtime services. > > Also require CAP_SYS_ADMIN to open the chardev to prevent unprivileged > users to call the EFI runtime services, instead of just relying on the > chardev file mode bits for this. > > The main user of this driver is the fwts [0] tool that already checks if > the effective user ID is 0 and fails otherwise. So this change shouldn't > cause any regression to this tool. > > [0]: https://wiki.ubuntu.com/FirmwareTestSuite/Reference/uefivarinfo > > Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> > Acked-by: Laszlo Ersek <lersek@xxxxxxxxxx> > > --- > > Changes in v2: > - Also disable /dev/efi_test access when the kernel is locked down as > suggested by Matthew Garrett. Right; if you remember the pre-patch discussion off-list, we kind of expected that lockdown might affect this. :) ... And, I can see Matt's comment now, at <https://bugzilla.redhat.com/show_bug.cgi?id=1759325#c1>. Thanks for that! While this change decreases the usability of the module, I fully agree it is justified for production use. While it's more convenient for me to keep SB enabled in the test VM(s) in general, and just run the test whenever I need it, security trumps convenience. I can disable SB when necessary, or even dedicate separate VMs (with SB generally disabled) to this kind of testing. > - Add Acked-by tag from Laszlo Ersek. My ACK stands -- I don't know enough to validate the security_locked_down() call and its friends, but I'm OK with the intent. Thanks all! Laszlo > > drivers/firmware/efi/test/efi_test.c | 8 ++++++++ > include/linux/security.h | 1 + > security/lockdown/lockdown.c | 1 + > 3 files changed, 10 insertions(+) > > diff --git a/drivers/firmware/efi/test/efi_test.c b/drivers/firmware/efi/test/efi_test.c > index 877745c3aaf..7baf48c01e7 100644 > --- a/drivers/firmware/efi/test/efi_test.c > +++ b/drivers/firmware/efi/test/efi_test.c > @@ -14,6 +14,7 @@ > #include <linux/init.h> > #include <linux/proc_fs.h> > #include <linux/efi.h> > +#include <linux/security.h> > #include <linux/slab.h> > #include <linux/uaccess.h> > > @@ -717,6 +718,13 @@ static long efi_test_ioctl(struct file *file, unsigned int cmd, > > static int efi_test_open(struct inode *inode, struct file *file) > { > + int ret = security_locked_down(LOCKDOWN_EFI_TEST); > + > + if (ret) > + return ret; > + > + if (!capable(CAP_SYS_ADMIN)) > + return -EACCES; > /* > * nothing special to do here > * We do accept multiple open files at the same time as we > diff --git a/include/linux/security.h b/include/linux/security.h > index a8d59d612d2..9df7547afc0 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -105,6 +105,7 @@ enum lockdown_reason { > LOCKDOWN_NONE, > LOCKDOWN_MODULE_SIGNATURE, > LOCKDOWN_DEV_MEM, > + LOCKDOWN_EFI_TEST, > LOCKDOWN_KEXEC, > LOCKDOWN_HIBERNATION, > LOCKDOWN_PCI_ACCESS, > diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c > index 8a10b43daf7..40b790536de 100644 > --- a/security/lockdown/lockdown.c > +++ b/security/lockdown/lockdown.c > @@ -20,6 +20,7 @@ static const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = { > [LOCKDOWN_NONE] = "none", > [LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading", > [LOCKDOWN_DEV_MEM] = "/dev/mem,kmem,port", > + [LOCKDOWN_EFI_TEST] = "/dev/efi_test access", > [LOCKDOWN_KEXEC] = "kexec of unsigned images", > [LOCKDOWN_HIBERNATION] = "hibernation", > [LOCKDOWN_PCI_ACCESS] = "direct PCI access", >
