[dpdk-dev,v2,2/4] net/mlx4: spawn rdma-core dependency plug-in
Checks
Commit Message
When mlx4 is not compiled directly as an independent shared object (e.g.
CONFIG_RTE_BUILD_SHARED_LIB not enabled for performance reasons), DPDK
applications inherit its dependencies on libibverbs and libmlx4 through
rte.app.mk.
This is an issue both when DPDK is delivered as a binary package (Linux
distributions) and for end users because rdma-core then propagates as a
mandatory dependency for everything.
Application writers relying on binary DPDK packages are not necessarily
aware of this fact and may end up delivering packages with broken
dependencies.
This patch therefore introduces an intermediate internal plug-in
hard-linked with rdma-core (to preserve symbol versioning) loaded by the
PMD through dlopen(), so that a missing rdma-core does not cause unresolved
symbols, allowing applications to start normally.
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
config/common_base | 1 +
doc/guides/nics/mlx4.rst | 13 +++++++
drivers/net/mlx4/Makefile | 41 ++++++++++++++++++++++
drivers/net/mlx4/mlx4.c | 80 ++++++++++++++++++++++++++++++++++++++++++
mk/rte.app.mk | 4 +++
5 files changed, 139 insertions(+)
Comments
On Fri, Jan 26, 2018 at 03:19:00PM +0100, Adrien Mazarguil wrote:
...
> +static int
> +mlx4_glue_init(void)
> +{
> + char file[] = "/tmp/" MLX4_DRIVER_NAME "_XXXXXX";
> + int fd = mkstemp(file);
...
> + while (off != mlx4_glue_lib_size) {
> + ssize_t ret;
> +
> + ret = write(fd, (const uint8_t *)mlx4_glue_lib + off,
> + mlx4_glue_lib_size - off);
> + if (ret == -1) {
> + if (errno != EINTR) {
> + rte_errno = errno;
> + goto glue_error;
> + }
> + ret = 0;
> + }
> + off += ret;
> + }
> + close(fd);
> + fd = -1;
> + handle = dlopen(file, RTLD_LAZY);
> + unlink(file);
This is a potential security issue. There are no guarantees that the
file dlopen() will open is the file that was just written above. It
could have been changed by something else in between.
Marcelo
Hi Marcelo,
Saturday, January 27, 2018 5:03 PM, Marcelo Ricardo Leitner:
> On Fri, Jan 26, 2018 at 03:19:00PM +0100, Adrien Mazarguil wrote:
> ...
> > +static int
> > +mlx4_glue_init(void)
> > +{
> > + char file[] = "/tmp/" MLX4_DRIVER_NAME "_XXXXXX";
> > + int fd = mkstemp(file);
> ...
> > + while (off != mlx4_glue_lib_size) {
> > + ssize_t ret;
> > +
> > + ret = write(fd, (const uint8_t *)mlx4_glue_lib + off,
> > + mlx4_glue_lib_size - off);
> > + if (ret == -1) {
> > + if (errno != EINTR) {
> > + rte_errno = errno;
> > + goto glue_error;
> > + }
> > + ret = 0;
> > + }
> > + off += ret;
> > + }
> > + close(fd);
> > + fd = -1;
> > + handle = dlopen(file, RTLD_LAZY);
> > + unlink(file);
>
> This is a potential security issue. There are no guarantees that the file
> dlopen() will open is the file that was just written above. It could have been
> changed by something else in between.
Can you further explain what are the potential risks you want to protect from?
I think this issue is not different from regular file protection under Linux.
If the DPDK process ran by root, then this approach is no less secure than the previous version of the patches that dlopen the /usr/lib/libibverbs.so and /usr/lib/libmlx5.so. root can also change them before the dlopen.
In fact in terms of security, root user can intentionally damage the system in many other ways.
If the DPDK process ran by regular user X, then the only users that are allowed to modify the file created are user X and possibly root. Other users will not have write permission to it.
if the same user change this temporary file, then it damages itself only, as the DPDK process run by it will probably won't lunch.
>
> Marcelo
Hi Shahaf,
On Sun, Jan 28, 2018 at 09:04:36AM +0000, Shahaf Shuler wrote:
> Hi Marcelo,
>
> Saturday, January 27, 2018 5:03 PM, Marcelo Ricardo Leitner:
> > On Fri, Jan 26, 2018 at 03:19:00PM +0100, Adrien Mazarguil wrote:
> > ...
> > > +static int
> > > +mlx4_glue_init(void)
> > > +{
> > > + char file[] = "/tmp/" MLX4_DRIVER_NAME "_XXXXXX";
> > > + int fd = mkstemp(file);
> > ...
> > > + while (off != mlx4_glue_lib_size) {
> > > + ssize_t ret;
> > > +
> > > + ret = write(fd, (const uint8_t *)mlx4_glue_lib + off,
> > > + mlx4_glue_lib_size - off);
> > > + if (ret == -1) {
> > > + if (errno != EINTR) {
> > > + rte_errno = errno;
> > > + goto glue_error;
> > > + }
> > > + ret = 0;
> > > + }
> > > + off += ret;
> > > + }
> > > + close(fd);
> > > + fd = -1;
> > > + handle = dlopen(file, RTLD_LAZY);
> > > + unlink(file);
> >
> > This is a potential security issue. There are no guarantees that the file
> > dlopen() will open is the file that was just written above. It could have been
> > changed by something else in between.
>
> Can you further explain what are the potential risks you want to
> protect from?
It is different in some aspects,
> I think this issue is not different from regular file protection
> under Linux.
in regular files we can ensure the right selinux contexts and
permissions are set, which is not the case here.
/usr could be even mounted as R/O and files just loaded from there,
while with this approach you're allocating a new file on a temporary
dir, which potentially is using extra RAM memory to store it and then
load it.
>
> If the DPDK process ran by root, then this approach is no less
> secure than the previous version of the patches that dlopen the
> /usr/lib/libibverbs.so and /usr/lib/libmlx5.so. root can also
> change them before the dlopen.
Maybe, and though that probably would leave traces in the system that
that happened.
> In fact in terms of security, root user can intentionally damage the
> system in many other ways.
And a SELinux (mis)configuration could grant (unexpected) write access
to the file for some other, non-root, user.
>
> If the DPDK process ran by regular user X, then the only users that
> are allowed to modify the file created are user X and possibly root.
> Other users will not have write permission to it.
> if the same user change this temporary file, then it damages itself
> only, as the DPDK process run by it will probably won't lunch.
Let's work this from the other PoV: why embed the library in dpdk
binary? How is it any more stable than regular libraries?
If it's just to hide it from rpm dependency generator, there is
probably a cleaner way to do it. For example, packaging the lib in a
sub-package, or maybe some more tricky rpm macro.
Marcelo
Sunday, January 28, 2018 1:18 PM, Marcelo Ricardo Leitner:
> Hi Shahaf,
>
> On Sun, Jan 28, 2018 at 09:04:36AM +0000, Shahaf Shuler wrote:
> > Hi Marcelo,
> >
> > Saturday, January 27, 2018 5:03 PM, Marcelo Ricardo Leitner:
> > > On Fri, Jan 26, 2018 at 03:19:00PM +0100, Adrien Mazarguil wrote:
> > > ...
> > > > +static int
> > > > +mlx4_glue_init(void)
> > > > +{
> > > > + char file[] = "/tmp/" MLX4_DRIVER_NAME "_XXXXXX";
> > > > + int fd = mkstemp(file);
> > > ...
> > > > + while (off != mlx4_glue_lib_size) {
> > > > + ssize_t ret;
> > > > +
> > > > + ret = write(fd, (const uint8_t *)mlx4_glue_lib + off,
> > > > + mlx4_glue_lib_size - off);
> > > > + if (ret == -1) {
> > > > + if (errno != EINTR) {
> > > > + rte_errno = errno;
> > > > + goto glue_error;
> > > > + }
> > > > + ret = 0;
> > > > + }
> > > > + off += ret;
> > > > + }
> > > > + close(fd);
> > > > + fd = -1;
> > > > + handle = dlopen(file, RTLD_LAZY);
> > > > + unlink(file);
> > >
> > > This is a potential security issue. There are no guarantees that the
> > > file
> > > dlopen() will open is the file that was just written above. It could
> > > have been changed by something else in between.
> >
> > Can you further explain what are the potential risks you want to
> > protect from?
>
> It is different in some aspects,
>
> > I think this issue is not different from regular file protection under
> > Linux.
>
> in regular files we can ensure the right selinux contexts and permissions are
> set, which is not the case here.
>
> /usr could be even mounted as R/O and files just loaded from there, while
> with this approach you're allocating a new file on a temporary dir, which
> potentially is using extra RAM memory to store it and then load it.
So the issue is with the absolute path that contains the temporary file or with the concept of embedding the library inside the DPDK binary?
>
> >
> > If the DPDK process ran by root, then this approach is no less secure
> > than the previous version of the patches that dlopen the
> > /usr/lib/libibverbs.so and /usr/lib/libmlx5.so. root can also change
> > them before the dlopen.
>
> Maybe, and though that probably would leave traces in the system that that
> happened.
>
> > In fact in terms of security, root user can intentionally damage the
> > system in many other ways.
>
> And a SELinux (mis)configuration could grant (unexpected) write access to
> the file for some other, non-root, user.
>
> >
> > If the DPDK process ran by regular user X, then the only users that
> > are allowed to modify the file created are user X and possibly root.
> > Other users will not have write permission to it.
> > if the same user change this temporary file, then it damages itself
> > only, as the DPDK process run by it will probably won't lunch.
>
> Let's work this from the other PoV: why embed the library in dpdk binary?
> How is it any more stable than regular libraries?
The main reason is not to force the DPDK user to install the DPDK.
Using the approach suggest on this commit, DPDK user can just "make" DPDK and that is it.
When the DPDK application starts, it spits a temporary file which happens to be usable with dlopen() and hard-linked with libibverbs and libmlx[45] and once loaded in memory, removes it from the file system.
dlopen the library itself will require the library to be in some well-known location.
>
> If it's just to hide it from rpm dependency generator, there is probably a
> cleaner way to do it. For example, packaging the lib in a sub-package, or
> maybe some more tricky rpm macro.
Do you see it as a preferred solution ? Having the glue lib included in rpms and install it per demand in case the binary execution failed?
>
> Marcelo
@@ -298,6 +298,7 @@ CONFIG_RTE_LIBRTE_AVF_16BYTE_RX_DESC=n
#
CONFIG_RTE_LIBRTE_MLX4_PMD=n
CONFIG_RTE_LIBRTE_MLX4_DEBUG=n
+CONFIG_RTE_LIBRTE_MLX4_DLOPEN_DEPS=y
CONFIG_RTE_LIBRTE_MLX4_TX_MP_CACHE=8
#
@@ -86,6 +86,19 @@ These options can be modified in the ``.config`` file.
Toggle compilation of librte_pmd_mlx4 itself.
+- ``CONFIG_RTE_LIBRTE_MLX4_DLOPEN_DEPS`` (default **y**)
+
+ Build PMD with additional code to make it loadable without hard
+ dependencies on **libibverbs** nor **libmlx4**, which may not be installed
+ on the target system.
+
+ In this mode, their presence is still required for it to run properly,
+ however their absence won't prevent a DPDK application from starting (with
+ ``CONFIG_RTE_BUILD_SHARED_LIB`` disabled) and they won't show up as
+ missing with ``ldd(1)``.
+
+ This option has no performance impact.
+
- ``CONFIG_RTE_LIBRTE_MLX4_DEBUG`` (default **n**)
Toggle debugging code and stricter compilation flags. Enabling this option
@@ -38,7 +38,11 @@ LIB = librte_pmd_mlx4.a
SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4.c
SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4_ethdev.c
SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4_flow.c
+ifeq ($(CONFIG_RTE_LIBRTE_MLX4_DLOPEN_DEPS),y)
+SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4_glue_lib.c
+else
SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4_glue.c
+endif
SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4_intr.c
SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4_mr.c
SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += mlx4_rxq.c
@@ -55,7 +59,12 @@ CFLAGS += -D_BSD_SOURCE
CFLAGS += -D_DEFAULT_SOURCE
CFLAGS += -D_XOPEN_SOURCE=600
CFLAGS += $(WERROR_FLAGS)
+ifeq ($(CONFIG_RTE_LIBRTE_MLX4_DLOPEN_DEPS),y)
+CFLAGS_mlx4_glue.o += -fPIC
+LDLIBS += -ldl
+else
LDLIBS += -libverbs -lmlx4
+endif
LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring
LDLIBS += -lrte_ethdev -lrte_net -lrte_kvargs
LDLIBS += -lrte_bus_pci
@@ -109,7 +118,39 @@ mlx4_autoconf.h: mlx4_autoconf.h.new
$(SRCS-$(CONFIG_RTE_LIBRTE_MLX4_PMD):.c=.o): mlx4_autoconf.h
+# Generate dependency plug-in for rdma-core when the PMD must not be linked
+# directly, so that applications do not inherit this dependency.
+
+ifeq ($(CONFIG_RTE_LIBRTE_MLX4_DLOPEN_DEPS),y)
+
+mlx4_glue_lib.c: mlx4_glue_lib.so
+ $Q printf '#include <stddef.h>\n' > $@
+ $Q printf '#include <stdint.h>\n\n' >> $@
+ $Q printf 'const uint8_t mlx4_glue_lib[][16] = {\n' >> $@
+ $Q od -vt x1 $< | \
+ sed -ne '/^[[:xdigit:]]\{1,\}/{' \
+ -e 's///;' \
+ -e '/^$$/d; ' \
+ -e 's/[[:space:]]*$$//;' \
+ -e 's/[[:space:]]\{1,\}/\\x/g;' \
+ -e 's/^/ "/;' \
+ -e 's/$$/",/;' \
+ -e 'p;' \
+ -e '}' >> $@
+ $Q printf '};\n\n' >> $@
+ $Q printf 'const size_t mlx4_glue_lib_size = %u;\n' \
+ $$(wc -c < $<) >> $@
+
+mlx4_glue_lib.so: mlx4_glue.o
+ $Q $(LD) $(LDFLAGS) $(EXTRA_LDFLAGS) \
+ -s -shared -o $@ $< -libverbs -lmlx4
+
+mlx4_glue.o: mlx4_autoconf.h
+
+endif
+
clean_mlx4: FORCE
$Q rm -f -- mlx4_autoconf.h mlx4_autoconf.h.new
+ $Q rm -f -- mlx4_glue.o mlx4_glue_lib.*
clean: clean_mlx4
@@ -37,6 +37,7 @@
*/
#include <assert.h>
+#include <dlfcn.h>
#include <errno.h>
#include <inttypes.h>
#include <stddef.h>
@@ -44,6 +45,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <unistd.h>
/* Verbs headers do not support -pedantic. */
#ifdef PEDANTIC
@@ -55,6 +57,7 @@
#endif
#include <rte_common.h>
+#include <rte_config.h>
#include <rte_dev.h>
#include <rte_errno.h>
#include <rte_ethdev_driver.h>
@@ -724,6 +727,78 @@ static struct rte_pci_driver mlx4_driver = {
RTE_PCI_DRV_INTR_RMV,
};
+#ifdef RTE_LIBRTE_MLX4_DLOPEN_DEPS
+
+extern const uint8_t mlx4_glue_lib[][16];
+extern const size_t mlx4_glue_lib_size;
+
+/**
+ * Initialization routine for run-time dependency on rdma-core.
+ */
+static int
+mlx4_glue_init(void)
+{
+ char file[] = "/tmp/" MLX4_DRIVER_NAME "_XXXXXX";
+ int fd = mkstemp(file);
+ size_t off = 0;
+ void *handle = NULL;
+ void **sym;
+ const char *dlmsg;
+
+ if (fd == -1) {
+ rte_errno = errno;
+ goto glue_error;
+ }
+ while (off != mlx4_glue_lib_size) {
+ ssize_t ret;
+
+ ret = write(fd, (const uint8_t *)mlx4_glue_lib + off,
+ mlx4_glue_lib_size - off);
+ if (ret == -1) {
+ if (errno != EINTR) {
+ rte_errno = errno;
+ goto glue_error;
+ }
+ ret = 0;
+ }
+ off += ret;
+ }
+ close(fd);
+ fd = -1;
+ handle = dlopen(file, RTLD_LAZY);
+ unlink(file);
+ if (!handle) {
+ rte_errno = EINVAL;
+ dlmsg = dlerror();
+ if (dlmsg)
+ ERROR("cannot load glue library: %s", dlmsg);
+ goto glue_error;
+ }
+ sym = dlsym(handle, "mlx4_glue");
+ if (!sym || !*sym) {
+ rte_errno = EINVAL;
+ dlmsg = dlerror();
+ if (dlmsg)
+ ERROR("cannot resolve glue symbol: %s", dlmsg);
+ goto glue_error;
+ }
+ mlx4_glue = *sym;
+ return 0;
+glue_error:
+ if (handle)
+ dlclose(handle);
+ if (fd != -1) {
+ close(fd);
+ unlink(file);
+ }
+ ERROR("cannot initialize PMD due to missing run-time"
+ " dependency on rdma-core libraries (libibverbs,"
+ " libmlx4)");
+ return -rte_errno;
+}
+
+#endif
+
/**
* Driver initialization routine.
*/
@@ -744,6 +819,11 @@ rte_mlx4_pmd_init(void)
* using this PMD, which is not supported in forked processes.
*/
setenv("RDMAV_HUGEPAGES_SAFE", "1", 1);
+#ifdef RTE_LIBRTE_MLX4_DLOPEN_DEPS
+ if (mlx4_glue_init())
+ return;
+ assert(mlx4_glue);
+#endif
mlx4_glue->fork_init();
rte_pci_register(&mlx4_driver);
}
@@ -143,7 +143,11 @@ ifeq ($(CONFIG_RTE_LIBRTE_KNI),y)
_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_KNI) += -lrte_pmd_kni
endif
_LDLIBS-$(CONFIG_RTE_LIBRTE_LIO_PMD) += -lrte_pmd_lio
+ifeq ($(CONFIG_RTE_LIBRTE_MLX4_DLOPEN_DEPS),y)
+_LDLIBS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += -lrte_pmd_mlx4 -ldl
+else
_LDLIBS-$(CONFIG_RTE_LIBRTE_MLX4_PMD) += -lrte_pmd_mlx4 -libverbs -lmlx4
+endif
_LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD) += -lrte_pmd_mlx5 -libverbs -lmlx5
_LDLIBS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) += -lrte_pmd_mrvl -L$(LIBMUSDK_PATH)/lib -lmusdk
_LDLIBS-$(CONFIG_RTE_LIBRTE_NFP_PMD) += -lrte_pmd_nfp