Re: [dpdk-dev] [PATCH v2 2/4] net/mlx4: spawn rdma-core dependency plug-in

[Shahaf Shuler <shahafs@mellanox.com>]

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