[dpdk-dev,v3] igb_uio: fail and log if kernel lock down is enabled

Message ID 20180516144220.21235-1-ferruh.yigit@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Ferruh Yigit May 16, 2018, 2:42 p.m. UTC
  When EFI secure boot is enabled, it is possible to lock down kernel and
prevent accessing device BARs and this makes igb_uio unusable.

Lock down patches are not part of the vanilla kernel but they are
applied and used by some distros already [1].

It is not possible to fix this issue, but intention of this patch is to
detect and log if kernel lock down enabled and don't insert the module
for that case.

The challenge is since this feature enabled by distros, they have
different config options and APIs for it. This patch is done based on
Fedora and Ubuntu kernel source, may needs to add more distro specific
support.

[1]
kernel.ubuntu.com/git/ubuntu/ubuntu-artful.git/commit/?id=99f9ef18d5b6
And a few more patches to

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Luca Boccassi <bluca@debian.org>
---
Cc: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Cc: Luca Boccassi <bluca@debian.org>
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: Neil Horman <nhorman@tuxdriver.com>
Cc: Stephen Hemminger <stephen@networkplumber.org>

v2:
* remove distro comments from checks
Note:
Since kernel_is_locked_down() is macro in one case, it can be used for
comparison:
 #ifdef kernel_is_locked_down
   kernel_is_locked_down(NULL)
 #else
   kernel_is_locked_down()

This will force all non macro defined cases to else and this may be
broken in the feature if macro changed.

To be more protective for changes, since this patch is not upstreamed to
kernel yet, will keep config check although it is ugly.

v3:
* remove git artifact, fix build
---
 kernel/linux/igb_uio/compat.h  | 23 +++++++++++++++++++----
 kernel/linux/igb_uio/igb_uio.c |  5 +++++
 2 files changed, 24 insertions(+), 4 deletions(-)
  

Comments

Neil Horman May 17, 2018, 11:34 a.m. UTC | #1
On Wed, May 16, 2018 at 03:42:20PM +0100, Ferruh Yigit wrote:
> When EFI secure boot is enabled, it is possible to lock down kernel and
> prevent accessing device BARs and this makes igb_uio unusable.
> 
> Lock down patches are not part of the vanilla kernel but they are
> applied and used by some distros already [1].
> 
> It is not possible to fix this issue, but intention of this patch is to
> detect and log if kernel lock down enabled and don't insert the module
> for that case.
> 
> The challenge is since this feature enabled by distros, they have
> different config options and APIs for it. This patch is done based on
> Fedora and Ubuntu kernel source, may needs to add more distro specific
> support.
> 
I still need to ask, what exactly is the error you're seeing with inserting the
uio module?  The lockdown patch set restricts BAR address changes, but via paths
acessible from user space, igbuio should still insert and initalize just fine
(or so it would seem to me).  Why not fix this by detecting the problem during
the user space library initalization, where you can do so via a standard method
that works accross distributions?

Neil
  
Ferruh Yigit May 17, 2018, 1:26 p.m. UTC | #2
On 5/17/2018 12:34 PM, Neil Horman wrote:
> On Wed, May 16, 2018 at 03:42:20PM +0100, Ferruh Yigit wrote:
>> When EFI secure boot is enabled, it is possible to lock down kernel and
>> prevent accessing device BARs and this makes igb_uio unusable.
>>
>> Lock down patches are not part of the vanilla kernel but they are
>> applied and used by some distros already [1].
>>
>> It is not possible to fix this issue, but intention of this patch is to
>> detect and log if kernel lock down enabled and don't insert the module
>> for that case.
>>
>> The challenge is since this feature enabled by distros, they have
>> different config options and APIs for it. This patch is done based on
>> Fedora and Ubuntu kernel source, may needs to add more distro specific
>> support.
>>
> I still need to ask, what exactly is the error you're seeing with inserting the
> uio module?  The lockdown patch set restricts BAR address changes, but via paths
> acessible from user space, igbuio should still insert and initalize just fine
> (or so it would seem to me).  Why not fix this by detecting the problem during
> the user space library initalization, where you can do so via a standard method
> that works accross distributions?

I have seen you comment on other thread, this v3 was just to fix a silly
mistake, lets continue discussion in other thread.
  
Neil Horman May 17, 2018, 6:16 p.m. UTC | #3
On Thu, May 17, 2018 at 02:26:12PM +0100, Ferruh Yigit wrote:
> On 5/17/2018 12:34 PM, Neil Horman wrote:
> > On Wed, May 16, 2018 at 03:42:20PM +0100, Ferruh Yigit wrote:
> >> When EFI secure boot is enabled, it is possible to lock down kernel and
> >> prevent accessing device BARs and this makes igb_uio unusable.
> >>
> >> Lock down patches are not part of the vanilla kernel but they are
> >> applied and used by some distros already [1].
> >>
> >> It is not possible to fix this issue, but intention of this patch is to
> >> detect and log if kernel lock down enabled and don't insert the module
> >> for that case.
> >>
> >> The challenge is since this feature enabled by distros, they have
> >> different config options and APIs for it. This patch is done based on
> >> Fedora and Ubuntu kernel source, may needs to add more distro specific
> >> support.
> >>
> > I still need to ask, what exactly is the error you're seeing with inserting the
> > uio module?  The lockdown patch set restricts BAR address changes, but via paths
> > acessible from user space, igbuio should still insert and initalize just fine
> > (or so it would seem to me).  Why not fix this by detecting the problem during
> > the user space library initalization, where you can do so via a standard method
> > that works accross distributions?
> 
> I have seen you comment on other thread, this v3 was just to fix a silly
> mistake, lets continue discussion in other thread.
> 
Copy that, thank you
Neil
  
Thomas Monjalon June 27, 2018, 2:39 p.m. UTC | #4
16/05/2018 16:42, Ferruh Yigit:
> When EFI secure boot is enabled, it is possible to lock down kernel and
> prevent accessing device BARs and this makes igb_uio unusable.
> 
> Lock down patches are not part of the vanilla kernel but they are
> applied and used by some distros already [1].
> 
> It is not possible to fix this issue, but intention of this patch is to
> detect and log if kernel lock down enabled and don't insert the module
> for that case.
> 
> The challenge is since this feature enabled by distros, they have
> different config options and APIs for it. This patch is done based on
> Fedora and Ubuntu kernel source, may needs to add more distro specific
> support.
> 
> [1]
> kernel.ubuntu.com/git/ubuntu/ubuntu-artful.git/commit/?id=99f9ef18d5b6
> And a few more patches to
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Luca Boccassi <bluca@debian.org>

Applied, thanks
  
David Marchand June 29, 2018, 7:04 a.m. UTC | #5
Hello Ferruh,

On Wed, May 16, 2018 at 4:42 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> @@ -136,3 +132,22 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 5, 0)
>  #define HAVE_PCI_MSI_MASK_IRQ 1
>  #endif
> +
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 8, 0)
> +#define HAVE_ALLOC_IRQ_VECTORS 1
> +#endif
> +
> +static inline bool igbuio_kernel_is_locked_down(void)
> +{
> +#ifdef CONFIG_LOCK_DOWN_KERNEL
> +#ifdef CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT
> +       return kernel_is_locked_down(NULL);
> +#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN
> +       return kernel_is_locked_down();
> +#else
> +       return false;
> +#endif
> +#else
> +       return false;
> +#endif
> +}

Missing some defined() for the check on CONFIG_EFI_SECURE_BOOT_LOCK_DOWN.
It causes a build error with ubuntu-16.04 hwe for aarch64.

I can send a patch.
  
Ferruh Yigit June 29, 2018, 9:35 a.m. UTC | #6
On 6/29/2018 8:04 AM, David Marchand wrote:
> Hello Ferruh,
> 
> On Wed, May 16, 2018 at 4:42 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> @@ -136,3 +132,22 @@ static bool pci_check_and_mask_intx(struct pci_dev *pdev)
>>  #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 5, 0)
>>  #define HAVE_PCI_MSI_MASK_IRQ 1
>>  #endif
>> +
>> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 8, 0)
>> +#define HAVE_ALLOC_IRQ_VECTORS 1
>> +#endif
>> +
>> +static inline bool igbuio_kernel_is_locked_down(void)
>> +{
>> +#ifdef CONFIG_LOCK_DOWN_KERNEL
>> +#ifdef CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT
>> +       return kernel_is_locked_down(NULL);
>> +#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN
>> +       return kernel_is_locked_down();
>> +#else
>> +       return false;
>> +#endif
>> +#else
>> +       return false;
>> +#endif
>> +}
> 
> Missing some defined() for the check on CONFIG_EFI_SECURE_BOOT_LOCK_DOWN.
> It causes a build error with ubuntu-16.04 hwe for aarch64.
> 
> I can send a patch.

Hi David,

You are right, please send the patch if have bandwidth for it, thanks.
  

Patch

diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
index d9f4d29fc..dfc66b1ef 100644
--- a/kernel/linux/igb_uio/compat.h
+++ b/kernel/linux/igb_uio/compat.h
@@ -125,10 +125,6 @@  static bool pci_check_and_mask_intx(struct pci_dev *pdev)
 #define HAVE_PCI_IS_BRIDGE_API 1
 #endif
 
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 8, 0)
-#define HAVE_ALLOC_IRQ_VECTORS 1
-#endif
-
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 3, 0)
 #define HAVE_MSI_LIST_IN_GENERIC_DEVICE 1
 #endif
@@ -136,3 +132,22 @@  static bool pci_check_and_mask_intx(struct pci_dev *pdev)
 #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 5, 0)
 #define HAVE_PCI_MSI_MASK_IRQ 1
 #endif
+
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 8, 0)
+#define HAVE_ALLOC_IRQ_VECTORS 1
+#endif
+
+static inline bool igbuio_kernel_is_locked_down(void)
+{
+#ifdef CONFIG_LOCK_DOWN_KERNEL
+#ifdef CONFIG_LOCK_DOWN_IN_EFI_SECURE_BOOT
+	return kernel_is_locked_down(NULL);
+#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN
+	return kernel_is_locked_down();
+#else
+	return false;
+#endif
+#else
+	return false;
+#endif
+}
diff --git a/kernel/linux/igb_uio/igb_uio.c b/kernel/linux/igb_uio/igb_uio.c
index cd9b7e721..b3233f18e 100644
--- a/kernel/linux/igb_uio/igb_uio.c
+++ b/kernel/linux/igb_uio/igb_uio.c
@@ -621,6 +621,11 @@  igbuio_pci_init_module(void)
 {
 	int ret;
 
+	if (igbuio_kernel_is_locked_down()) {
+		pr_err("Not able to use module, kernel lock down is enabled\n");
+		return -EINVAL;
+	}
+
 	ret = igbuio_config_intr_mode(intr_mode);
 	if (ret < 0)
 		return ret;