[PATCH] igb_uio: fail and log if kernel lock down is enabled

[PATCH] igb_uio: fail and log if kernel lock down is enabled

[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>
---
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>
---
 kernel/linux/igb_uio/compat.h  | 24 ++++++++++++++++++++----
 kernel/linux/igb_uio/igb_uio.c |  5 +++++
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
index d9f4d29fc..774c980c2 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,23 @@ 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 /* fedora */
+	return kernel_is_locked_down(NULL);
+#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN /* ubuntu */
+	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;
-- 
2.14.3

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Luca Boccassi]

On Tue, 2018-05-15 at 17:56 +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.
> 
> [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>
> ---
> 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>
> ---
>  kernel/linux/igb_uio/compat.h  | 24 ++++++++++++++++++++----
>  kernel/linux/igb_uio/igb_uio.c |  5 +++++
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/compat.h
> b/kernel/linux/igb_uio/compat.h
> index d9f4d29fc..774c980c2 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,23 @@ 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 /* fedora */
> +	return kernel_is_locked_down(NULL);
> +#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN /* ubuntu */
> +	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;

kernel_is_locked_down already does print a message, so it seems a bit
redundant (you can call it with something
like kernel_is_locked_down("DPDK igb_uio kernel module")).

In Debian Stretch the patches is the same as Ubuntu (Securelevel) but
it didn't ship with any signed binaries so it's unlikely to be used.
In Debian Buster the patchset is the same as Fedora (Lockdown).

-- 
Kind regards,
Luca Boccassi

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Stephen Hemminger]

On Tue, 15 May 2018 17:56:12 +0100
Ferruh Yigit <ferruh.yigit@intel.com> 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.

This distro specific (and not upstream) stuff in igb_uio is getting
to be a significantly ugly. Can this be detected by seeing if one
of the calls (such setup bars) failing?

The best long term solution would be getting all kernel modules upstream;
but it looks like that will never happen due to UIO maintainer resistance.

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Ferruh Yigit]

On 5/15/2018 6:47 PM, Luca Boccassi wrote:
> On Tue, 2018-05-15 at 17:56 +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.
>>
>> [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>
>> ---
>> 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>
>> ---
>>  kernel/linux/igb_uio/compat.h  | 24 ++++++++++++++++++++----
>>  kernel/linux/igb_uio/igb_uio.c |  5 +++++
>>  2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/linux/igb_uio/compat.h
>> b/kernel/linux/igb_uio/compat.h
>> index d9f4d29fc..774c980c2 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,23 @@ 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 /* fedora */
>> +	return kernel_is_locked_down(NULL);
>> +#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN /* ubuntu */
>> +	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;
> 
> kernel_is_locked_down already does print a message, so it seems a bit
> redundant (you can call it with something
> like kernel_is_locked_down("DPDK igb_uio kernel module")).

Yes, and no. There are two version of kernel_is_locked_down(), macro one gets
the string argument, the functions one doesn't get any paramter.

Two unify the message I think it is better to prevent kernel message by
providing NULL variable to macro and print log in igb_uio.


btw, I was thinking kernel_is_locked_down() difference is between fedora and
ubuntu but it is not, it seems related to the kernel version as well.

ubuntu-bionic (4.15.0) is using LOCK_DOWN_IN_EFI_SECURE_BOOT config option and
kernel_is_locked_down(char *msg) macro,
ubuntu-artful (4.13.16) is using EFI_SECURE_BOOT_LOCK_DOWN config option and
kernel_is_locked_down(void) function.

I will update the patch according.

> 
> In Debian Stretch the patches is the same as Ubuntu (Securelevel) but
> it didn't ship with any signed binaries so it's unlikely to be used.
> In Debian Buster the patchset is the same as Fedora (Lockdown).
> 

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Ferruh Yigit]

On 5/15/2018 7:52 PM, Stephen Hemminger wrote:
> On Tue, 15 May 2018 17:56:12 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> 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.
> 
> This distro specific (and not upstream) stuff in igb_uio is getting
> to be a significantly ugly. Can this be detected by seeing if one
> of the calls (such setup bars) failing?

Agreed that it is ugly.
igb_uio already will fail in these systems, we can detect failure but the target
is detect reason of failure and inform user about root cause.
I can't think of any other way than using distro specific stuff for this case.

> 
> The best long term solution would be getting all kernel modules upstream;
> but it looks like that will never happen due to UIO maintainer resistance.
> 

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Luca Boccassi]

On Wed, 2018-05-16 at 10:45 +0100, Ferruh Yigit wrote:
> On 5/15/2018 6:47 PM, Luca Boccassi wrote:
> > On Tue, 2018-05-15 at 17:56 +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.
> > > 
> > > [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>
> > > ---
> > > 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>
> > > ---
> > >  kernel/linux/igb_uio/compat.h  | 24 ++++++++++++++++++++----
> > >  kernel/linux/igb_uio/igb_uio.c |  5 +++++
> > >  2 files changed, 25 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/kernel/linux/igb_uio/compat.h
> > > b/kernel/linux/igb_uio/compat.h
> > > index d9f4d29fc..774c980c2 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,23 @@ 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 /* fedora */
> > > +	return kernel_is_locked_down(NULL);
> > > +#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN /* ubuntu */
> > > +	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;
> > 
> > kernel_is_locked_down already does print a message, so it seems a
> > bit
> > redundant (you can call it with something
> > like kernel_is_locked_down("DPDK igb_uio kernel module")).
> 
> Yes, and no. There are two version of kernel_is_locked_down(), macro
> one gets
> the string argument, the functions one doesn't get any paramter.
> 
> Two unify the message I think it is better to prevent kernel message
> by
> providing NULL variable to macro and print log in igb_uio.
> 
> 
> btw, I was thinking kernel_is_locked_down() difference is between
> fedora and
> ubuntu but it is not, it seems related to the kernel version as well.
> 
> ubuntu-bionic (4.15.0) is using LOCK_DOWN_IN_EFI_SECURE_BOOT config
> option and
> kernel_is_locked_down(char *msg) macro,
> ubuntu-artful (4.13.16) is using EFI_SECURE_BOOT_LOCK_DOWN config
> option and
> kernel_is_locked_down(void) function.
> 
> I will update the patch according.

Yes, they all use the same patchset, which originally was called
"securelevel" and was the first version used initially when distros
started to ship with secure boot support.
In the past couple of years it's been reworked and it's now called
"lockdown".

Expect all distros to move to the newer version at some point in time.

-- 
Kind regards,
Luca Boccassi

[PATCH v2] igb_uio: fail and log if kernel lock down is enabled

[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>
---
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.
---
 kernel/linux/igb_uio/compat.h  | 25 +++++++++++++++++++++----
 kernel/linux/igb_uio/igb_uio.c |  5 +++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
index d9f4d29fc..5befec588 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,24 @@ 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 kernel_is_locked_down
+#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;
-- 
2.14.3

Re: [PATCH v2] igb_uio: fail and log if kernel lock down is enabled

[Luca Boccassi]

On Wed, 2018-05-16 at 11:18 +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.
> 
> [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>
> ---
> 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.
> ---

Acked-by: Luca Boccassi <bluca@debian.org>

-- 
Kind regards,
Luca Boccassi

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Neil Horman]

On Tue, May 15, 2018 at 05:56:12PM +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.
> 
> [1]
> kernel.ubuntu.com/git/ubuntu/ubuntu-artful.git/commit/?id=99f9ef18d5b6
> And a few more patches to
> 
What exactly is the error you get when you load the igb_uio module?  I ask
because, looking at least at the Fedora patches, the BAR registers themselves
aren't made unwriteable, its only userspace access through very specific
channels that are gated on (things like /proc/bus/pci/...).  From what I can see
(again, not having looked at other implementations), kernel modules that load
successfully should be able to modify bar registers, and otherwise function
normally (as to weather they are permitted to load is another question).

The reason I ask this is twofold:

1) if a specific access is failing, that seems like it could be the trigger to
use, rather than explicitly checking if the kernel is locked down.  I don't see
one expressly called, but if you're calling pci_write_config_* somewhere, and
getting an EPERM error, thats a reason to fail the loading of igb_uio, based on
the fact that you don't have permission to write to the appropriate hardware.

2) Its more than just the igb_uio module that will fail.  Any attempt to pass a
VF into a guest using user space tools (including the vfio scripts that dpdk
includes), should fail.  As such, it might be better to have some component in
user space test one of the aforementioned restricted paths for writeability.
Such an approach would be more generic, and eliminate the need to assemble a set
of tests to see if the kernel is locked down.  A more generic error message
could then be logged and the dpdk could exit gracefully, weather or not igb_uio
was loaded.

Its probably also important to note here that, this lockdown patch, from my
digging, has been carried in Fedora since December of 2016, and its still not
made it upstream.  Thats not to say that it will never do so, but it suggests
that, given the 2 years of out of tree updates its received, there its use is
both very specific and limted to users who understand its implications.  This
probably isn't something to make significant or hard-to-maintain changes to the
dpdk (or any other software) over.

Neil

> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> 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>
> ---
>  kernel/linux/igb_uio/compat.h  | 24 ++++++++++++++++++++----
>  kernel/linux/igb_uio/igb_uio.c |  5 +++++
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
> index d9f4d29fc..774c980c2 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,23 @@ 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 /* fedora */
> +	return kernel_is_locked_down(NULL);
> +#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN /* ubuntu */
> +	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;
> -- 
> 2.14.3
> 
> 

[PATCH v3] igb_uio: fail and log if kernel lock down is enabled

[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>
---
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(-)

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;
-- 
2.14.3

Re: [PATCH v3] igb_uio: fail and log if kernel lock down is enabled

[Neil Horman]

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

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Ferruh Yigit]

On 5/16/2018 12:47 PM, Neil Horman wrote:
> On Tue, May 15, 2018 at 05:56:12PM +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.
>>
>> [1]
>> kernel.ubuntu.com/git/ubuntu/ubuntu-artful.git/commit/?id=99f9ef18d5b6
>> And a few more patches to
>>
> What exactly is the error you get when you load the igb_uio module?  I ask
> because, looking at least at the Fedora patches, the BAR registers themselves
> aren't made unwriteable, its only userspace access through very specific
> channels that are gated on (things like /proc/bus/pci/...).  From what I can see
> (again, not having looked at other implementations), kernel modules that load
> successfully should be able to modify bar registers, and otherwise function
> normally (as to weather they are permitted to load is another question).

This patch is based on understanding on the effect of the lockdown patches, that
it will disable hardware access from userspace.
I don't have an environment to test this and indeed I am not very clear about
effects of the lockdown set.

> 
> The reason I ask this is twofold:
> 
> 1) if a specific access is failing, that seems like it could be the trigger to
> use, rather than explicitly checking if the kernel is locked down.  I don't see
> one expressly called, but if you're calling pci_write_config_* somewhere, and
> getting an EPERM error, thats a reason to fail the loading of igb_uio, based on
> the fact that you don't have permission to write to the appropriate hardware.
> 
> 2) Its more than just the igb_uio module that will fail.  Any attempt to pass a
> VF into a guest using user space tools (including the vfio scripts that dpdk
> includes), should fail.  As such, it might be better to have some component in
> user space test one of the aforementioned restricted paths for writeability.
> Such an approach would be more generic, and eliminate the need to assemble a set
> of tests to see if the kernel is locked down.  A more generic error message
> could then be logged and the dpdk could exit gracefully, weather or not igb_uio
> was loaded.

With the existing patches, expectation is vfio will work but it will only effect
igb_uio.

> 
> Its probably also important to note here that, this lockdown patch, from my
> digging, has been carried in Fedora since December of 2016, and its still not
> made it upstream.  Thats not to say that it will never do so, but it suggests
> that, given the 2 years of out of tree updates its received, there its use is
> both very specific and limted to users who understand its implications.  This
> probably isn't something to make significant or hard-to-maintain changes to the
> dpdk (or any other software) over.

Have same expectation that use will be specific and limited, that is why planed
to change only igb_uio to detect the case and return with a log, instead of
updating anything in the dpdk.

in igb_uio the plan was just adding simple check, patches being not upstreamed
added more complexity, but not still I believe it is not significant or
hard-to-maintain change.

> 
> Neil
> 
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> 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>
>> ---
>>  kernel/linux/igb_uio/compat.h  | 24 ++++++++++++++++++++----
>>  kernel/linux/igb_uio/igb_uio.c |  5 +++++
>>  2 files changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/linux/igb_uio/compat.h b/kernel/linux/igb_uio/compat.h
>> index d9f4d29fc..774c980c2 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,23 @@ 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 /* fedora */
>> +	return kernel_is_locked_down(NULL);
>> +#elif CONFIG_EFI_SECURE_BOOT_LOCK_DOWN /* ubuntu */
>> +	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;
>> -- 
>> 2.14.3
>>
>>

Re: [PATCH v3] igb_uio: fail and log if kernel lock down is enabled

[Ferruh Yigit]

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.

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Stephen Hemminger]

On Thu, 17 May 2018 14:23:46 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 5/16/2018 12:47 PM, Neil Horman wrote:
> > On Tue, May 15, 2018 at 05:56:12PM +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.
> >>
> >> [1]
> >> kernel.ubuntu.com/git/ubuntu/ubuntu-artful.git/commit/?id=99f9ef18d5b6
> >> And a few more patches to
> >>  
> > What exactly is the error you get when you load the igb_uio module?  I ask
> > because, looking at least at the Fedora patches, the BAR registers themselves
> > aren't made unwriteable, its only userspace access through very specific
> > channels that are gated on (things like /proc/bus/pci/...).  From what I can see
> > (again, not having looked at other implementations), kernel modules that load
> > successfully should be able to modify bar registers, and otherwise function
> > normally (as to weather they are permitted to load is another question).  
> 
> This patch is based on understanding on the effect of the lockdown patches, that
> it will disable hardware access from userspace.
> I don't have an environment to test this and indeed I am not very clear about
> effects of the lockdown set.
> 
> > 
> > The reason I ask this is twofold:
> > 
> > 1) if a specific access is failing, that seems like it could be the trigger to
> > use, rather than explicitly checking if the kernel is locked down.  I don't see
> > one expressly called, but if you're calling pci_write_config_* somewhere, and
> > getting an EPERM error, thats a reason to fail the loading of igb_uio, based on
> > the fact that you don't have permission to write to the appropriate hardware.
> > 
> > 2) Its more than just the igb_uio module that will fail.  Any attempt to pass a
> > VF into a guest using user space tools (including the vfio scripts that dpdk
> > includes), should fail.  As such, it might be better to have some component in
> > user space test one of the aforementioned restricted paths for writeability.
> > Such an approach would be more generic, and eliminate the need to assemble a set
> > of tests to see if the kernel is locked down.  A more generic error message
> > could then be logged and the dpdk could exit gracefully, weather or not igb_uio
> > was loaded.  
> 
> With the existing patches, expectation is vfio will work but it will only effect
> igb_uio.
> 
> > 
> > Its probably also important to note here that, this lockdown patch, from my
> > digging, has been carried in Fedora since December of 2016, and its still not
> > made it upstream.  Thats not to say that it will never do so, but it suggests
> > that, given the 2 years of out of tree updates its received, there its use is
> > both very specific and limted to users who understand its implications.  This
> > probably isn't something to make significant or hard-to-maintain changes to the
> > dpdk (or any other software) over.  
> 
> Have same expectation that use will be specific and limited, that is why planed
> to change only igb_uio to detect the case and return with a log, instead of
> updating anything in the dpdk.
> 
> in igb_uio the plan was just adding simple check, patches being not upstreamed
> added more complexity, but not still I believe it is not significant or
> hard-to-maintain change.

The  issue is that igb_uio is not secure since it allows userspace to setup
DMA to any physical address. In lockdown mode, even root is not supposed to be
able to peek and poke arbitrary memory.

Actually, it would make more sense to just have code to block all UIO drivers
in uio.c since uio_pci_generic has the same issue.

Re: [PATCH v3] igb_uio: fail and log if kernel lock down is enabled

[Neil Horman]

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

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Neil Horman]

On Thu, May 17, 2018 at 07:39:12AM -0700, Stephen Hemminger wrote:
> On Thu, 17 May 2018 14:23:46 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> > On 5/16/2018 12:47 PM, Neil Horman wrote:
> > > On Tue, May 15, 2018 at 05:56:12PM +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.
> > >>
> > >> [1]
> > >> kernel.ubuntu.com/git/ubuntu/ubuntu-artful.git/commit/?id=99f9ef18d5b6
> > >> And a few more patches to
> > >>  
> > > What exactly is the error you get when you load the igb_uio module?  I ask
> > > because, looking at least at the Fedora patches, the BAR registers themselves
> > > aren't made unwriteable, its only userspace access through very specific
> > > channels that are gated on (things like /proc/bus/pci/...).  From what I can see
> > > (again, not having looked at other implementations), kernel modules that load
> > > successfully should be able to modify bar registers, and otherwise function
> > > normally (as to weather they are permitted to load is another question).  
> > 
> > This patch is based on understanding on the effect of the lockdown patches, that
> > it will disable hardware access from userspace.
> > I don't have an environment to test this and indeed I am not very clear about
> > effects of the lockdown set.
> > 
> > > 
> > > The reason I ask this is twofold:
> > > 
> > > 1) if a specific access is failing, that seems like it could be the trigger to
> > > use, rather than explicitly checking if the kernel is locked down.  I don't see
> > > one expressly called, but if you're calling pci_write_config_* somewhere, and
> > > getting an EPERM error, thats a reason to fail the loading of igb_uio, based on
> > > the fact that you don't have permission to write to the appropriate hardware.
> > > 
> > > 2) Its more than just the igb_uio module that will fail.  Any attempt to pass a
> > > VF into a guest using user space tools (including the vfio scripts that dpdk
> > > includes), should fail.  As such, it might be better to have some component in
> > > user space test one of the aforementioned restricted paths for writeability.
> > > Such an approach would be more generic, and eliminate the need to assemble a set
> > > of tests to see if the kernel is locked down.  A more generic error message
> > > could then be logged and the dpdk could exit gracefully, weather or not igb_uio
> > > was loaded.  
> > 
> > With the existing patches, expectation is vfio will work but it will only effect
> > igb_uio.
> > 
> > > 
> > > Its probably also important to note here that, this lockdown patch, from my
> > > digging, has been carried in Fedora since December of 2016, and its still not
> > > made it upstream.  Thats not to say that it will never do so, but it suggests
> > > that, given the 2 years of out of tree updates its received, there its use is
> > > both very specific and limted to users who understand its implications.  This
> > > probably isn't something to make significant or hard-to-maintain changes to the
> > > dpdk (or any other software) over.  
> > 
> > Have same expectation that use will be specific and limited, that is why planed
> > to change only igb_uio to detect the case and return with a log, instead of
> > updating anything in the dpdk.
> > 
> > in igb_uio the plan was just adding simple check, patches being not upstreamed
> > added more complexity, but not still I believe it is not significant or
> > hard-to-maintain change.
> 
> The  issue is that igb_uio is not secure since it allows userspace to setup
> DMA to any physical address. In lockdown mode, even root is not supposed to be
> able to peek and poke arbitrary memory.
> 
> Actually, it would make more sense to just have code to block all UIO drivers
> in uio.c since uio_pci_generic has the same issue.
> 
That makes a bit more sense to me, yes.
Neil

Re: [PATCH] igb_uio: fail and log if kernel lock down is enabled

[Ferruh Yigit]

On 5/17/2018 8:49 PM, Neil Horman wrote:
> On Thu, May 17, 2018 at 07:39:12AM -0700, Stephen Hemminger wrote:
>> On Thu, 17 May 2018 14:23:46 +0100
>> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>>> On 5/16/2018 12:47 PM, Neil Horman wrote:
>>>> On Tue, May 15, 2018 at 05:56:12PM +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.
>>>>>
>>>>> [1]
>>>>> kernel.ubuntu.com/git/ubuntu/ubuntu-artful.git/commit/?id=99f9ef18d5b6
>>>>> And a few more patches to
>>>>>  
>>>> What exactly is the error you get when you load the igb_uio module?  I ask
>>>> because, looking at least at the Fedora patches, the BAR registers themselves
>>>> aren't made unwriteable, its only userspace access through very specific
>>>> channels that are gated on (things like /proc/bus/pci/...).  From what I can see
>>>> (again, not having looked at other implementations), kernel modules that load
>>>> successfully should be able to modify bar registers, and otherwise function
>>>> normally (as to weather they are permitted to load is another question).  
>>>
>>> This patch is based on understanding on the effect of the lockdown patches, that
>>> it will disable hardware access from userspace.
>>> I don't have an environment to test this and indeed I am not very clear about
>>> effects of the lockdown set.
>>>
>>>>
>>>> The reason I ask this is twofold:
>>>>
>>>> 1) if a specific access is failing, that seems like it could be the trigger to
>>>> use, rather than explicitly checking if the kernel is locked down.  I don't see
>>>> one expressly called, but if you're calling pci_write_config_* somewhere, and
>>>> getting an EPERM error, thats a reason to fail the loading of igb_uio, based on
>>>> the fact that you don't have permission to write to the appropriate hardware.
>>>>
>>>> 2) Its more than just the igb_uio module that will fail.  Any attempt to pass a
>>>> VF into a guest using user space tools (including the vfio scripts that dpdk
>>>> includes), should fail.  As such, it might be better to have some component in
>>>> user space test one of the aforementioned restricted paths for writeability.
>>>> Such an approach would be more generic, and eliminate the need to assemble a set
>>>> of tests to see if the kernel is locked down.  A more generic error message
>>>> could then be logged and the dpdk could exit gracefully, weather or not igb_uio
>>>> was loaded.  
>>>
>>> With the existing patches, expectation is vfio will work but it will only effect
>>> igb_uio.
>>>
>>>>
>>>> Its probably also important to note here that, this lockdown patch, from my
>>>> digging, has been carried in Fedora since December of 2016, and its still not
>>>> made it upstream.  Thats not to say that it will never do so, but it suggests
>>>> that, given the 2 years of out of tree updates its received, there its use is
>>>> both very specific and limted to users who understand its implications.  This
>>>> probably isn't something to make significant or hard-to-maintain changes to the
>>>> dpdk (or any other software) over.  
>>>
>>> Have same expectation that use will be specific and limited, that is why planed
>>> to change only igb_uio to detect the case and return with a log, instead of
>>> updating anything in the dpdk.
>>>
>>> in igb_uio the plan was just adding simple check, patches being not upstreamed
>>> added more complexity, but not still I believe it is not significant or
>>> hard-to-maintain change.
>>
>> The  issue is that igb_uio is not secure since it allows userspace to setup
>> DMA to any physical address. In lockdown mode, even root is not supposed to be
>> able to peek and poke arbitrary memory.
>>
>> Actually, it would make more sense to just have code to block all UIO drivers
>> in uio.c since uio_pci_generic has the same issue.
>>
> That makes a bit more sense to me, yes.

Hi Thomas,

We can postpone this to next release, no need to get any risk related this for
this release.

Thanks,
ferruh

Re: [PATCH v3] igb_uio: fail and log if kernel lock down is enabled

[Thomas Monjalon]

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

Re: [PATCH v3] igb_uio: fail and log if kernel lock down is enabled

[David Marchand]

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.

-- 
David Marchand

Re: [PATCH v3] igb_uio: fail and log if kernel lock down is enabled

[Ferruh Yigit]

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.