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