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

[Marcelo Ricardo Leitner <mleitner@redhat.com>]

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