DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org,
	Christian Ehrhardt <christian.ehrhardt@canonical.com>,
	Luca Boccassi <bluca@debian.org>,
	Maxime Coquelin <maxime.coquelin@redhat.com>,
	Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [dpdk-dev] [PATCH] igb_uio: fail and log if kernel lock down is enabled
Date: Wed, 16 May 2018 07:47:15 -0400	[thread overview]
Message-ID: <20180516114715.GA20004@hmswarspite.think-freely.org> (raw)
In-Reply-To: <20180515165612.61243-1-ferruh.yigit@intel.com>

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

  parent reply	other threads:[~2018-05-16 11:48 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15 16:56 Ferruh Yigit
2018-05-15 17:47 ` Luca Boccassi
2018-05-16  9:45   ` Ferruh Yigit
2018-05-16  9:56     ` Luca Boccassi
2018-05-15 18:52 ` Stephen Hemminger
2018-05-16  9:53   ` Ferruh Yigit
2018-05-16 10:18 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2018-05-16 10:50   ` Luca Boccassi
2018-05-16 14:42   ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
2018-05-17 11:34     ` Neil Horman
2018-05-17 13:26       ` Ferruh Yigit
2018-05-17 18:16         ` Neil Horman
2018-06-27 14:39     ` Thomas Monjalon
2018-06-29  7:04     ` David Marchand
2018-06-29  9:35       ` Ferruh Yigit
2018-05-16 11:47 ` Neil Horman [this message]
2018-05-17 13:23   ` [dpdk-dev] [PATCH] " Ferruh Yigit
2018-05-17 14:39     ` Stephen Hemminger
2018-05-17 19:49       ` Neil Horman
2018-05-22 15:23         ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180516114715.GA20004@hmswarspite.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=bluca@debian.org \
    --cc=christian.ehrhardt@canonical.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=stephen@networkplumber.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).