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
