[dpdk-dev] [PATCH 0/4] improve runtime loading of shared drivers

[dpdk-dev] [PATCH 0/4] improve runtime loading of shared drivers

[Bruce Richardson]

This set includes a number of small improvements for handling the loading
of drivers at runtime using the EAL -d flag.

It limits the loading of files to only those files which end in .so, which
means that one can pass in the whole "drivers/" subfolder from a meson
build and not get an error when DPDK trys to load a .a file.

It also puts in some basic permission checking to ensure that no drivers
are loaded from a world-writable location on the filesystem, which would be
a potential security hole on a mis-configured system.

Bruce Richardson (4):
  eal: remove unnecessary null-termination
  eal: only load shared libs from driver plugin directory
  eal: don't load drivers from insecure paths
  eal: cache last directory permissions checked

 lib/librte_eal/common/eal_common_options.c | 91 ++++++++++++++++++++--
 1 file changed, 83 insertions(+), 8 deletions(-)

-- 
2.25.1

[dpdk-dev] [PATCH 1/4] eal: remove unnecessary null-termination

[Bruce Richardson]

Since strlcpy always null-terminates, and the buffer is zeroed before copy
anyway, there is no need to explicitly zero the end of the character
array, or to limit the bytes that strlcpy can write.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 8f2cbd1c6..6fbe9b5db 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -322,8 +322,7 @@ eal_plugin_add(const char *path)
 		return -1;
 	}
 	memset(solib, 0, sizeof(*solib));
-	strlcpy(solib->name, path, PATH_MAX-1);
-	solib->name[PATH_MAX-1] = 0;
+	strlcpy(solib->name, path, PATH_MAX);
 	TAILQ_INSERT_TAIL(&solib_list, solib, next);
 
 	return 0;
-- 
2.25.1

[dpdk-dev] [PATCH 2/4] eal: only load shared libs from driver plugin directory

[Bruce Richardson]

When we pass a "-d" flag to EAL pointing to a directory, we attempt to load
all files in that directory as driver plugins, irrespective of file type.
This procludes using e.g. the build/drivers directory, as a driver source
since it contains static libs and other files as well as the shared
objects.

By filtering out any files whose filename does not end in ".so", we can
improve usability by allowing other non-driver files to be present in the
driver directory.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 6fbe9b5db..7aef6df4c 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -347,9 +347,15 @@ eal_plugindir_init(const char *path)
 
 	while ((dent = readdir(d)) != NULL) {
 		struct stat sb;
+		int nlen = strnlen(dent->d_name, sizeof(dent->d_name));
+
+		/* check if name ends in .so */
+		if (strcmp(&dent->d_name[nlen - 3], ".so") != 0)
+			continue;
 
 		snprintf(sopath, sizeof(sopath), "%s/%s", path, dent->d_name);
 
+		/* if a regular file, add to list to load */
 		if (!(stat(sopath, &sb) == 0 && S_ISREG(sb.st_mode)))
 			continue;
 
-- 
2.25.1

[dpdk-dev] [PATCH 3/4] eal: don't load drivers from insecure paths

[Bruce Richardson]

Any paths on the system which are world-writable are insecure and should
not be used for loading drivers. Therefore check each driver path before
loading it and error out on insecure ones.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 75 ++++++++++++++++++++--
 1 file changed, 69 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 7aef6df4c..2a62a1342 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -15,6 +15,7 @@
 #include <getopt.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <dlfcn.h>
+#include <libgen.h>
 #endif
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -368,10 +369,74 @@ eal_plugindir_init(const char *path)
 	return (dent == NULL) ? 0 : -1;
 }
 
+#ifdef RTE_EXEC_ENV_WINDOWS
+int
+eal_plugins_init(void)
+{
+	return 0;
+}
+#else
+
+static int
+verify_perms(const char *dirpath)
+{
+	struct stat st;
+
+	/* if not root, check down one level first */
+	if (strcmp(dirpath, "/") != 0) {
+		char copy[PATH_MAX];
+
+		strlcpy(copy, dirpath, PATH_MAX);
+		if (verify_perms(dirname(copy)) != 0)
+			return -1;
+	}
+
+	/* call stat to check for permissions and ensure not world writable */
+	if (stat(dirpath, &st) != 0) {
+		RTE_LOG(ERR, EAL, "Error with stat on %s, %s\n",
+				dirpath, strerror(errno));
+		return -1;
+	}
+	if (st.st_mode & S_IWOTH) {
+		RTE_LOG(ERR, EAL,
+				"Error, directory path %s is world-writable and insecure\n",
+				dirpath);
+		return -1;
+	}
+
+	return 0;
+}
+
+static void *
+eal_dlopen(const char *pathname)
+{
+	void *retval = NULL;
+	char *realp = realpath(pathname, NULL);
+
+	if (realp == NULL) {
+		RTE_LOG(ERR, EAL, "Error with realpath, %s\n", strerror(errno));
+		goto out;
+	}
+	if (strnlen(realp, PATH_MAX) == PATH_MAX) {
+		RTE_LOG(ERR, EAL, "Error, driver path greater than PATH_MAX\n");
+		goto out;
+	}
+
+	/* do permissions checks */
+	if (verify_perms(realp) != 0)
+		goto out;
+
+	retval = dlopen(realp, RTLD_NOW);
+	if (retval == NULL)
+		RTE_LOG(ERR, EAL, "%s\n", dlerror());
+out:
+	free(realp);
+	return retval;
+}
+
 int
 eal_plugins_init(void)
 {
-#ifndef RTE_EXEC_ENV_WINDOWS
 	struct shared_driver *solib = NULL;
 	struct stat sb;
 
@@ -391,17 +456,15 @@ eal_plugins_init(void)
 		} else {
 			RTE_LOG(DEBUG, EAL, "open shared lib %s\n",
 				solib->name);
-			solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-			if (solib->lib_handle == NULL) {
-				RTE_LOG(ERR, EAL, "%s\n", dlerror());
+			solib->lib_handle = eal_dlopen(solib->name);
+			if (solib->lib_handle == NULL)
 				return -1;
-			}
 		}
 
 	}
 	return 0;
-#endif
 }
+#endif
 
 /*
  * Parse the coremask given as argument (hexadecimal string) and fill
-- 
2.25.1

[dpdk-dev] [PATCH 4/4] eal: cache last directory permissions checked

[Bruce Richardson]

When loading a directory of drivers, we check the same hierarchy multiple
times. If we just cache the last directory checked, this avoids repeated
checks of the same path, since all drivers in that path have been added to
the list consecutively.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 2a62a1342..0901b493c 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -384,11 +384,18 @@ verify_perms(const char *dirpath)
 
 	/* if not root, check down one level first */
 	if (strcmp(dirpath, "/") != 0) {
+		static __thread char last_dir_checked[PATH_MAX];
 		char copy[PATH_MAX];
+		const char *dir;
 
 		strlcpy(copy, dirpath, PATH_MAX);
-		if (verify_perms(dirname(copy)) != 0)
-			return -1;
+		dir = dirname(copy);
+		if (strncmp(dir, last_dir_checked, PATH_MAX) != 0) {
+			if (verify_perms(dir) != 0)
+				return -1;
+			else
+				strlcpy(last_dir_checked, dir, PATH_MAX);
+		}
 	}
 
 	/* call stat to check for permissions and ensure not world writable */
-- 
2.25.1

[dpdk-dev] [PATCH v2 0/4] improve runtime loading of shared drivers

[Bruce Richardson]

This set includes a number of small improvements for handling the loading
of drivers at runtime using the EAL -d flag.

It limits the loading of files to only those files which end in .so, which
means that one can pass in the whole "drivers/" subfolder from a meson
build and not get an error when DPDK trys to load a .a file.

It also puts in some basic permission checking to ensure that no drivers
are loaded from a world-writable location on the filesystem, which would be
a potential security hole on a mis-configured system.

v2: rebased to fix errors on apply
    fixed one checkpatch issue.

Bruce Richardson (4):
  eal: remove unnecessary null-termination
  eal: only load shared libs from driver plugin directory
  eal: don't load drivers from insecure paths
  eal: cache last directory permissions checked

 lib/librte_eal/common/eal_common_options.c | 92 +++++++++++++++++++---
 1 file changed, 82 insertions(+), 10 deletions(-)

-- 
2.25.1

[dpdk-dev] [PATCH v2 1/4] eal: remove unnecessary null-termination

[Bruce Richardson]

Since strlcpy always null-terminates, and the buffer is zeroed before copy
anyway, there is no need to explicitly zero the end of the character
array, or to limit the bytes that strlcpy can write.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 0546beb3a..551507af1 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -326,8 +326,7 @@ eal_plugin_add(const char *path)
 		return -1;
 	}
 	memset(solib, 0, sizeof(*solib));
-	strlcpy(solib->name, path, PATH_MAX-1);
-	solib->name[PATH_MAX-1] = 0;
+	strlcpy(solib->name, path, PATH_MAX);
 	TAILQ_INSERT_TAIL(&solib_list, solib, next);
 
 	return 0;
-- 
2.25.1

[dpdk-dev] [PATCH v2 2/4] eal: only load shared libs from driver plugin directory

[Bruce Richardson]

When we pass a "-d" flag to EAL pointing to a directory, we attempt to load
all files in that directory as driver plugins, irrespective of file type.
This procludes using e.g. the build/drivers directory, as a driver source
since it contains static libs and other files as well as the shared
objects.

By filtering out any files whose filename does not end in ".so", we can
improve usability by allowing other non-driver files to be present in the
driver directory.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 551507af1..1a836d70f 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -352,9 +352,15 @@ eal_plugindir_init(const char *path)
 
 	while ((dent = readdir(d)) != NULL) {
 		struct stat sb;
+		int nlen = strnlen(dent->d_name, sizeof(dent->d_name));
+
+		/* check if name ends in .so */
+		if (strcmp(&dent->d_name[nlen - 3], ".so") != 0)
+			continue;
 
 		snprintf(sopath, sizeof(sopath), "%s/%s", path, dent->d_name);
 
+		/* if a regular file, add to list to load */
 		if (!(stat(sopath, &sb) == 0 && S_ISREG(sb.st_mode)))
 			continue;
 
-- 
2.25.1

[dpdk-dev] [PATCH v2 3/4] eal: don't load drivers from insecure paths

[Bruce Richardson]

Any paths on the system which are world-writable are insecure and should
not be used for loading drivers. Therefore check each driver path before
loading it and error out on insecure ones.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v2: rebased to latest head to fix errors on apply in CI testing
---
 lib/librte_eal/common/eal_common_options.c | 77 +++++++++++++++++++---
 1 file changed, 69 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 1a836d70f..6978e6744 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -15,6 +15,7 @@
 #include <getopt.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <dlfcn.h>
+#include <libgen.h>
 #endif
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -332,7 +333,14 @@ eal_plugin_add(const char *path)
 	return 0;
 }
 
-#ifndef RTE_EXEC_ENV_WINDOWS
+#ifdef RTE_EXEC_ENV_WINDOWS
+int
+eal_plugins_init(void)
+{
+	return 0;
+}
+#else
+
 static int
 eal_plugindir_init(const char *path)
 {
@@ -372,12 +380,67 @@ eal_plugindir_init(const char *path)
 	/* XXX this ignores failures from readdir() itself */
 	return (dent == NULL) ? 0 : -1;
 }
-#endif
+
+static int
+verify_perms(const char *dirpath)
+{
+	struct stat st;
+
+	/* if not root, check down one level first */
+	if (strcmp(dirpath, "/") != 0) {
+		char copy[PATH_MAX];
+
+		strlcpy(copy, dirpath, PATH_MAX);
+		if (verify_perms(dirname(copy)) != 0)
+			return -1;
+	}
+
+	/* call stat to check for permissions and ensure not world writable */
+	if (stat(dirpath, &st) != 0) {
+		RTE_LOG(ERR, EAL, "Error with stat on %s, %s\n",
+				dirpath, strerror(errno));
+		return -1;
+	}
+	if (st.st_mode & S_IWOTH) {
+		RTE_LOG(ERR, EAL,
+				"Error, directory path %s is world-writable and insecure\n",
+				dirpath);
+		return -1;
+	}
+
+	return 0;
+}
+
+static void *
+eal_dlopen(const char *pathname)
+{
+	void *retval = NULL;
+	char *realp = realpath(pathname, NULL);
+
+	if (realp == NULL) {
+		RTE_LOG(ERR, EAL, "Error with realpath, %s\n", strerror(errno));
+		goto out;
+	}
+	if (strnlen(realp, PATH_MAX) == PATH_MAX) {
+		RTE_LOG(ERR, EAL, "Error, driver path greater than PATH_MAX\n");
+		goto out;
+	}
+
+	/* do permissions checks */
+	if (verify_perms(realp) != 0)
+		goto out;
+
+	retval = dlopen(realp, RTLD_NOW);
+	if (retval == NULL)
+		RTE_LOG(ERR, EAL, "%s\n", dlerror());
+out:
+	free(realp);
+	return retval;
+}
 
 int
 eal_plugins_init(void)
 {
-#ifndef RTE_EXEC_ENV_WINDOWS
 	struct shared_driver *solib = NULL;
 	struct stat sb;
 
@@ -397,17 +460,15 @@ eal_plugins_init(void)
 		} else {
 			RTE_LOG(DEBUG, EAL, "open shared lib %s\n",
 				solib->name);
-			solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-			if (solib->lib_handle == NULL) {
-				RTE_LOG(ERR, EAL, "%s\n", dlerror());
+			solib->lib_handle = eal_dlopen(solib->name);
+			if (solib->lib_handle == NULL)
 				return -1;
-			}
 		}
 
 	}
-#endif
 	return 0;
 }
+#endif
 
 /*
  * Parse the coremask given as argument (hexadecimal string) and fill
-- 
2.25.1

[dpdk-dev] [PATCH v2 4/4] eal: cache last directory permissions checked

[Bruce Richardson]

When loading a directory of drivers, we check the same hierarchy multiple
times. If we just cache the last directory checked, this avoids repeated
checks of the same path, since all drivers in that path have been added to
the list consecutively.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
V2: Fix checkpatch issue for unnecessary else.
---
 lib/librte_eal/common/eal_common_options.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 6978e6744..e6b2a195f 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -388,11 +388,17 @@ verify_perms(const char *dirpath)
 
 	/* if not root, check down one level first */
 	if (strcmp(dirpath, "/") != 0) {
+		static __thread char last_dir_checked[PATH_MAX];
 		char copy[PATH_MAX];
+		const char *dir;
 
 		strlcpy(copy, dirpath, PATH_MAX);
-		if (verify_perms(dirname(copy)) != 0)
-			return -1;
+		dir = dirname(copy);
+		if (strncmp(dir, last_dir_checked, PATH_MAX) != 0) {
+			if (verify_perms(dir) != 0)
+				return -1;
+			strlcpy(last_dir_checked, dir, PATH_MAX);
+		}
 	}
 
 	/* call stat to check for permissions and ensure not world writable */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v2 0/4] improve runtime loading of shared drivers

[Thomas Monjalon]

22/06/2020 16:33, Bruce Richardson:
> This set includes a number of small improvements for handling the loading
> of drivers at runtime using the EAL -d flag.
> 
> It limits the loading of files to only those files which end in .so, which
> means that one can pass in the whole "drivers/" subfolder from a meson
> build and not get an error when DPDK trys to load a .a file.
> 
> It also puts in some basic permission checking to ensure that no drivers
> are loaded from a world-writable location on the filesystem, which would be
> a potential security hole on a mis-configured system.
> 
> v2: rebased to fix errors on apply
>     fixed one checkpatch issue.
> 
> Bruce Richardson (4):
>   eal: remove unnecessary null-termination
>   eal: only load shared libs from driver plugin directory
>   eal: don't load drivers from insecure paths
>   eal: cache last directory permissions checked

There is an error when running devtools/test-null.sh:

EAL: Error with realpath, No such file or directory
EAL: FATAL: Cannot init plugins

Re: [dpdk-dev] [PATCH v2 0/4] improve runtime loading of shared drivers

[Thomas Monjalon]

22/06/2020 16:33, Bruce Richardson:
> Bruce Richardson (4):
>   eal: remove unnecessary null-termination

Maybe add scope of the change with "in plugin path" ?

>   eal: only load shared libs from driver plugin directory

I suggest: "eal: load only shared libraries from plugin directory"

>   eal: don't load drivers from insecure paths

I don't know why, I don't like titles starting with "don't".
I suggest: "eal: forbid plugin from insecure path"

>   eal: cache last directory permissions checked

Re: [dpdk-dev] [PATCH v2 0/4] improve runtime loading of shared drivers

[Bruce Richardson]

On Thu, Jul 02, 2020 at 11:16:51PM +0200, Thomas Monjalon wrote:
> 22/06/2020 16:33, Bruce Richardson:
> > Bruce Richardson (4):
> >   eal: remove unnecessary null-termination
> 
> Maybe add scope of the change with "in plugin path" ?
> 
> >   eal: only load shared libs from driver plugin directory
> 
> I suggest: "eal: load only shared libraries from plugin directory"
> 
> >   eal: don't load drivers from insecure paths
> 
> I don't know why, I don't like titles starting with "don't".
> I suggest: "eal: forbid plugin from insecure path"
> 
> >   eal: cache last directory permissions checked
> 
Will adjust for v3

[dpdk-dev] [PATCH v3 0/4] improve runtime loading of shared drivers

[Bruce Richardson]

This set includes a number of small improvements for handling the loading
of drivers at runtime using the EAL -d flag.

It limits the loading of files to only those files which end in .so, which
means that one can pass in the whole "drivers/" subfolder from a meson
build and not get an error when DPDK trys to load a .a file.

It also puts in some basic permission checking to ensure that no drivers
are loaded from a world-writable location on the filesystem, which would be
a potential security hole on a mis-configured system.

v3: adjusted commit titles based on Thomas' feedback
    skip over any paths which are not relative/absolute - assume any found
    from system directories by linker must be secure.

v2: rebased to fix errors on apply
    fixed one checkpatch issue.

Bruce Richardson (4):
  eal: remove unnecessary null-termination in plugin path
  eal: load only shared libs from driver plugin directories
  eal: forbid loading drivers from insecure paths
  eal: cache last directory permissions checked

 lib/librte_eal/common/eal_common_options.c | 100 ++++++++++++++++++---
 1 file changed, 90 insertions(+), 10 deletions(-)

-- 
2.25.1

[dpdk-dev] [PATCH v3 1/4] eal: remove unnecessary null-termination in plugin path

[Bruce Richardson]

Since strlcpy always null-terminates, and the buffer is zeroed before copy
anyway, there is no need to explicitly zero the end of the character
array, or to limit the bytes that strlcpy can write.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 24b223ebf..75e8839c3 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -352,8 +352,7 @@ eal_plugin_add(const char *path)
 		return -1;
 	}
 	memset(solib, 0, sizeof(*solib));
-	strlcpy(solib->name, path, PATH_MAX-1);
-	solib->name[PATH_MAX-1] = 0;
+	strlcpy(solib->name, path, PATH_MAX);
 	TAILQ_INSERT_TAIL(&solib_list, solib, next);
 
 	return 0;
-- 
2.25.1

[dpdk-dev] [PATCH v3 2/4] eal: load only shared libs from driver plugin directories

[Bruce Richardson]

When we pass a "-d" flag to EAL pointing to a directory, we attempt to load
all files in that directory as driver plugins, irrespective of file type.
This procludes using e.g. the build/drivers directory, as a driver source
since it contains static libs and other files as well as the shared
objects.

By filtering out any files whose filename does not end in ".so", we can
improve usability by allowing other non-driver files to be present in the
driver directory.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 75e8839c3..176a98561 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -378,9 +378,15 @@ eal_plugindir_init(const char *path)
 
 	while ((dent = readdir(d)) != NULL) {
 		struct stat sb;
+		int nlen = strnlen(dent->d_name, sizeof(dent->d_name));
+
+		/* check if name ends in .so */
+		if (strcmp(&dent->d_name[nlen - 3], ".so") != 0)
+			continue;
 
 		snprintf(sopath, sizeof(sopath), "%s/%s", path, dent->d_name);
 
+		/* if a regular file, add to list to load */
 		if (!(stat(sopath, &sb) == 0 && S_ISREG(sb.st_mode)))
 			continue;
 
-- 
2.25.1

[dpdk-dev] [PATCH v3 3/4] eal: forbid loading drivers from insecure paths

[Bruce Richardson]

Any paths on the system which are world-writable are insecure and should
not be used for loading drivers. Therefore, whenever an absolute or
relative driver path is passed to EAL, check for world-writability and
don't load any drivers from that path if it is insecure. Drivers loaded
from system locations i.e. those passed without any path info and found
automatically by the loader, are excluded from these checks as system paths
are assumed to be secure.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
v3: add exception for case where we don't have a relative/absolute
    path we can access. Just assume system directories are secure.
---
 lib/librte_eal/common/eal_common_options.c | 85 ++++++++++++++++++++--
 1 file changed, 77 insertions(+), 8 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 176a98561..6c63b9364 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -15,6 +15,7 @@
 #include <getopt.h>
 #ifndef RTE_EXEC_ENV_WINDOWS
 #include <dlfcn.h>
+#include <libgen.h>
 #endif
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -358,7 +359,14 @@ eal_plugin_add(const char *path)
 	return 0;
 }
 
-#ifndef RTE_EXEC_ENV_WINDOWS
+#ifdef RTE_EXEC_ENV_WINDOWS
+int
+eal_plugins_init(void)
+{
+	return 0;
+}
+#else
+
 static int
 eal_plugindir_init(const char *path)
 {
@@ -398,12 +406,75 @@ eal_plugindir_init(const char *path)
 	/* XXX this ignores failures from readdir() itself */
 	return (dent == NULL) ? 0 : -1;
 }
-#endif
+
+static int
+verify_perms(const char *dirpath)
+{
+	struct stat st;
+
+	/* if not root, check down one level first */
+	if (strcmp(dirpath, "/") != 0) {
+		char copy[PATH_MAX];
+
+		strlcpy(copy, dirpath, PATH_MAX);
+		if (verify_perms(dirname(copy)) != 0)
+			return -1;
+	}
+
+	/* call stat to check for permissions and ensure not world writable */
+	if (stat(dirpath, &st) != 0) {
+		RTE_LOG(ERR, EAL, "Error with stat on %s, %s\n",
+				dirpath, strerror(errno));
+		return -1;
+	}
+	if (st.st_mode & S_IWOTH) {
+		RTE_LOG(ERR, EAL,
+				"Error, directory path %s is world-writable and insecure\n",
+				dirpath);
+		return -1;
+	}
+
+	return 0;
+}
+
+static void *
+eal_dlopen(const char *pathname)
+{
+	void *retval = NULL;
+	char *realp = realpath(pathname, NULL);
+
+	if (realp == NULL && errno == ENOENT) {
+		/* not a full or relative path, try a load from system dirs */
+		retval = dlopen(pathname, RTLD_NOW);
+		if (retval == NULL)
+			RTE_LOG(ERR, EAL, "%s\n", dlerror());
+		return retval;
+	}
+	if (realp == NULL) {
+		RTE_LOG(ERR, EAL, "Error with realpath for %s, %s\n",
+				pathname, strerror(errno));
+		goto out;
+	}
+	if (strnlen(realp, PATH_MAX) == PATH_MAX) {
+		RTE_LOG(ERR, EAL, "Error, driver path greater than PATH_MAX\n");
+		goto out;
+	}
+
+	/* do permissions checks */
+	if (verify_perms(realp) != 0)
+		goto out;
+
+	retval = dlopen(realp, RTLD_NOW);
+	if (retval == NULL)
+		RTE_LOG(ERR, EAL, "%s\n", dlerror());
+out:
+	free(realp);
+	return retval;
+}
 
 int
 eal_plugins_init(void)
 {
-#ifndef RTE_EXEC_ENV_WINDOWS
 	struct shared_driver *solib = NULL;
 	struct stat sb;
 
@@ -423,17 +494,15 @@ eal_plugins_init(void)
 		} else {
 			RTE_LOG(DEBUG, EAL, "open shared lib %s\n",
 				solib->name);
-			solib->lib_handle = dlopen(solib->name, RTLD_NOW);
-			if (solib->lib_handle == NULL) {
-				RTE_LOG(ERR, EAL, "%s\n", dlerror());
+			solib->lib_handle = eal_dlopen(solib->name);
+			if (solib->lib_handle == NULL)
 				return -1;
-			}
 		}
 
 	}
-#endif
 	return 0;
 }
+#endif
 
 /*
  * Parse the coremask given as argument (hexadecimal string) and fill
-- 
2.25.1

[dpdk-dev] [PATCH v3 4/4] eal: cache last directory permissions checked

[Bruce Richardson]

When loading a directory of drivers, we check the same hierarchy multiple
times. If we just cache the last directory checked, this avoids repeated
checks of the same path, since all drivers in that path have been added to
the list consecutively.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_options.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 6c63b9364..85d5ba723 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -414,11 +414,17 @@ verify_perms(const char *dirpath)
 
 	/* if not root, check down one level first */
 	if (strcmp(dirpath, "/") != 0) {
+		static __thread char last_dir_checked[PATH_MAX];
 		char copy[PATH_MAX];
+		const char *dir;
 
 		strlcpy(copy, dirpath, PATH_MAX);
-		if (verify_perms(dirname(copy)) != 0)
-			return -1;
+		dir = dirname(copy);
+		if (strncmp(dir, last_dir_checked, PATH_MAX) != 0) {
+			if (verify_perms(dir) != 0)
+				return -1;
+			strlcpy(last_dir_checked, dir, PATH_MAX);
+		}
 	}
 
 	/* call stat to check for permissions and ensure not world writable */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v2 0/4] improve runtime loading of shared drivers

[Bruce Richardson]

On Thu, Jul 02, 2020 at 11:13:02PM +0200, Thomas Monjalon wrote:
> 22/06/2020 16:33, Bruce Richardson:
> > This set includes a number of small improvements for handling the loading
> > of drivers at runtime using the EAL -d flag.
> > 
> > It limits the loading of files to only those files which end in .so, which
> > means that one can pass in the whole "drivers/" subfolder from a meson
> > build and not get an error when DPDK trys to load a .a file.
> > 
> > It also puts in some basic permission checking to ensure that no drivers
> > are loaded from a world-writable location on the filesystem, which would be
> > a potential security hole on a mis-configured system.
> > 
> > v2: rebased to fix errors on apply
> >     fixed one checkpatch issue.
> > 
> > Bruce Richardson (4):
> >   eal: remove unnecessary null-termination
> >   eal: only load shared libs from driver plugin directory
> >   eal: don't load drivers from insecure paths
> >   eal: cache last directory permissions checked
> 
> There is an error when running devtools/test-null.sh:
> 
> EAL: Error with realpath, No such file or directory
> EAL: FATAL: Cannot init plugins
> 
Yes, I missed the fact that we can load drivers without paths letting
dlopen search system directories. I think we can assume system dirs are
secure, and so can just skip any permission checks in case where we can't
get the realpath of the filename passed in.

Fixed in v3.

Re: [dpdk-dev] [PATCH v3 0/4] improve runtime loading of shared drivers

[Thomas Monjalon]

03/07/2020 12:23, Bruce Richardson:
> This set includes a number of small improvements for handling the loading
> of drivers at runtime using the EAL -d flag.
> 
> It limits the loading of files to only those files which end in .so, which
> means that one can pass in the whole "drivers/" subfolder from a meson
> build and not get an error when DPDK trys to load a .a file.
> 
> It also puts in some basic permission checking to ensure that no drivers
> are loaded from a world-writable location on the filesystem, which would be
> a potential security hole on a mis-configured system.
> 
> v3: adjusted commit titles based on Thomas' feedback
>     skip over any paths which are not relative/absolute - assume any found
>     from system directories by linker must be secure.
> 
> v2: rebased to fix errors on apply
>     fixed one checkpatch issue.
> 
> Bruce Richardson (4):
>   eal: remove unnecessary null-termination in plugin path
>   eal: load only shared libs from driver plugin directories
>   eal: forbid loading drivers from insecure paths
>   eal: cache last directory permissions checked

Applied, thanks