[SYCL] Add implicit rpath to libsycl directory

[npmiller]

This patch adds the flag -f{no-}sycl-implicit-rpath, which ensures that on Linux-like platforms the directory containing libsycl.so in the clang build or installation is added to the SYCL binaries' RPATH. This flag is enabled by default.

This makes it easier for SYCL binaries to find libsycl.so and removes the need for user to set LD_LIBRARY_PATH in most cases.

This flag is similar to the OpenMP -fopenmp-implicit-rpath flag.

This patch is related to: https://github.com/intel/llvm/issues/6301 and in that case should make the extra CMake configuration unnecessary as well.

[bader]

I thought the main reason rpath hasn't been added before is security related risks: https://security.stackexchange.com/a/165762. I would be great if someone can comment if this patch is safe in that regard.

[npmiller]

That's a very interesting question, just a few notes on that:

• This patch doesn't add RPATH to any binaries that would end up in the DPC++ packages for two reasons:
  • It only adds it when compiling binaries in SYCL mode with clang++
  • CMake strips all the RPATH at the installation step by default

So as far as I understand it this would rather come into play when users of DPC++ are packaging SYCL applications. In that scenario if they don't use -fno-sycl-implicit-rpath or a build system such as CMake which strips RPATH in the installation step they may get exposed to the attacks outlined in the article.

Note that in CMake for example when using target_link_libraries it will by default add the directory containing the relevant library to the RUNPATH of the executable. This is why for example in a DPC++ build, you don't need to set LD_LIBRARY_PATH when calling sycl-ls from the build directory, but you do need it if you do ninja install and try to call sycl-ls from the installation package, as CMake will have stripped the RUNPATH.

Overall I think that this patch brings a better development environment in line with what CMake usually does, and that ultimately it is up to packagers of SYCL applications to make sure they don't have any RPATH to the build directories left in their packages, by either using the -fno-sycl-implicit-rpath flag or a build system like CMake which will strip it away during installation.

But I suppose an argument can be made that making this the default may cause application developers to accidentally ship DPC++ SYCL applications with unwanted RPATH to their build directories.

[bader]

making this the default may cause application developers to accidentally ship DPC++ SYCL applications with unwanted RPATH to their build directories.

I think that's the main point of the concern.

[elizabethandrews]

making this the default may cause application developers to accidentally ship DPC++ SYCL applications with unwanted RPATH to their build directories.

I think that's the main point of the concern.

Why are we making this default behavior?

[npmiller]

The main idea of this patch is to make it a bit more user friendly to use the SYCL compiler by removing the need to set LD_LIBRARY_PATH to run samples that were just built by clang++. So adding the flag as disabled by default could be helpful but wouldn't be as user-friendly.

Assuming a DPC++ compiler installed in /opt/llvm it's the difference between doing:

With this patch:

/opt/llvm/bin/clang++ -fsycl sample.cpp -o sample
./sample

With this patch but flag disabled by default:

/opt/llvm/bin/clang++ -fsycl -fsycl-implicit-rpath sample.cpp -o sample
./sample

Without this patch (or with this patch but flag disabled by default):

/opt/llvm/bin/clang++ -fsycl sample.cpp -o sample
export LD_LIBRARY_PATH=/opt/llvm/lib
./sample

So the tradeoff is that with this patch developers won't ever have to worry about their SYCL applications not finding libsycl.so, but packagers will need to ensure the rpath is stripped from the SYCL binaries they ship.

[elizabethandrews]

The main idea of this patch is to make it a bit more user friendly to use the SYCL compiler by removing the need to set LD_LIBRARY_PATH to run samples that were just built by clang++. So adding the flag as disabled by default could be helpful but wouldn't be as user-friendly.

Assuming a DPC++ compiler installed in /opt/llvm it's the difference between doing:

With this patch:

/opt/llvm/bin/clang++ -fsycl sample.cpp -o sample
./sample

With this patch but flag disabled by default:

/opt/llvm/bin/clang++ -fsycl -fsycl-implicit-rpath sample.cpp -o sample
./sample

Without this patch (or with this patch but flag disabled by default):

/opt/llvm/bin/clang++ -fsycl sample.cpp -o sample
export LD_LIBRARY_PATH=/opt/llvm/lib
./sample

So the tradeoff is that with this patch developers won't ever have to worry about their SYCL applications not finding libsycl.so, but packagers will need to ensure the rpath is stripped from the SYCL binaries they ship.

I understand but if security is a concern, maybe making this disabled by default is a better option even if it is not as user-friendly, I am not a security expert, so I am hoping someone else can comment @AaronBallman is this something you are familiar with?

[AaronBallman]

I understand but if security is a concern, maybe making this disabled by default is a better option even if it is not as user-friendly, I am not a security expert, so I am hoping someone else can comment @AaronBallman is this something you are familiar with?

Unfortunately, I don't feel confident I have enough expertise to answer this.

[erichkeane]

I think even making this NOT a default is a concern. Having dynamic libraries pulled in from directories I dont set in my OS by programs I otherwise would trust, is frightening. Unless my OS explicitly prompted me to let it load these, I'd be very unhappy with the implications of this. Even if it DID explicitly prompt me, I'd still be concerned.

[pvchupin]

@npmiller, have there been any discussions/concerns raised when -fopenmp-implicit-rpath was added in upstream? I assume we just trying to follow same path here?

[cperkinsintel]

To effectively use SYCL on Linux the user needs to not only have the SYCL shared library on the LD_LIBRARY_PATH, but also the device compilers. To my mind, while putting the shared lib on the LD_LIBRARY_PATH is arguably inconvenient, at least it's consistent with the device drivers. If the SYCL shared library is magically handled for the user but the device compilers are not, then I'm not sure we are gaining much. And that's before we address the security concern. RPATH hides from the user this important (and potentially exploitable) aspect of their built app.

[npmiller]

@npmiller, have there been any discussions/concerns raised when -fopenmp-implicit-rpath was added in upstream? I assume we just trying to follow same path here?

Here's the upstream discussion when adding the OpenMP flag:

• https://reviews.llvm.org/D118493

They didn't discuss much the security aspects of it but did mention that some distributions banned the use of RPATH and the conclusion was that packagers would just use the flag to disable it when building OpenMP applications.

To effectively use SYCL on Linux the user needs to not only have the SYCL shared library on the LD_LIBRARY_PATH, but also the device compilers. To my mind, while putting the shared lib on the LD_LIBRARY_PATH is arguably inconvenient, at least it's consistent with the device drivers. If the SYCL shared library is magically handled for the user but the device compilers are not, then I'm not sure we are gaining much. And that's before we address the security concern. RPATH hides from the user this important (and potentially exploitable) aspect of their built app.

What do you mean exactly by device compilers/drivers? The PI plugins are in the same directory as libsycl.so so they also benefit from the rpath, and for vendor libraries such as CUDA, HIP or level zero, you do have a point, but I think these are more likely to be installed on the system so they may not require LD_LIBRARY_PATH as much, and even if they are missing the SYCL application will still "work", as they're loaded dynamically, it just won't be able to use that specific plugin, which is arguably better.

It's true that RPATH hides some of that from the user, but at the same time -fsycl hides the fact that we're linking against libsycl.so to begin with. And using RPATH to resolve library dependencies in the development environment is what OpenMP does currently, and it's also the default behavior in CMake builds.

Talking about CMake, maybe as an alternative to this patch we could disable the flag by default but provide a DPC++ CMake configuration that would enable this rpath flag whenever appropriate so that it matches the usual CMake behavior. I think that would be an improvement while still being fairly conservative with it. Although I do think there is still a pretty good case to be made for enabling it by default, but I can see it both ways.

[npmiller]

The PI plugins are in the same directory as libsycl.so so they also benefit from the rpath

I just realized this is incorrect, I was just experimenting with a build directory and so libsycl.so also had an rpath for the current directory, so that's why it was able to find the plugins. You're absolutely right that when using an installation of DPC++ just having the rpath on the SYCL binary is not enough for libsycl.so to also find the plugins.

There's actually a recent ticket about exactly that issue here: https://github.com/intel/llvm/issues/6217 also suggesting to set a relative rpath on libsycl.so so that it can find the plugins. And apparently it's also mentioned as a TBD in the plugin documentation.

[rolandschulz]

I don't see how this could introduce any potential security issue. If someone doesn't trust our compiler or the location they install our compiler/runtime, they can't trust a binary produced with that compiler. In other words if you sideload an untrustworthy compiler (or binary produced by one) you have much bigger problems then rpath. And if they do trust our compiler (and the installation folder) why would the rpath cause any additional attack surface?

[AaronBallman]

I don't see how this could introduce any potential security issue.

https://security.stackexchange.com/questions/161799/why-does-checksec-sh-highlight-rpath-and-runpath-as-security-issues

If someone doesn't trust our compiler or the location they install our compiler/runtime, they can't trust a binary produced with that compiler.

It's not about the location of where they install the our compiler, it's that other things on the system (which the user may or may not have control over) may surprisingly change our compiler's behavior because we configured our compiler in a way that allows for that.

[rolandschulz]

It's not about the location of where they install the our compiler, it's that other things on the system (which the user may or may not have control over) may surprisingly change our compiler's behavior because we configured our compiler in a way that allows for that.

I assume you mean the application behavior. Because the compiler behavior would be unaffected because this PR is adding the rpath to the application compiled by the compiler not to the compiler itself.

The rpath would be pointing to the compiler installation location. This means that only modifying libraries inside the compilation installation location would modify the application behavior.

[AaronBallman]

It's not about the location of where they install the our compiler, it's that other things on the system (which the user may or may not have control over) may surprisingly change our compiler's behavior because we configured our compiler in a way that allows for that.

I assume you mean the application behavior. Because the compiler behavior would be unaffected because this PR is adding the rpath to the application compiled by the compiler not to the compiler itself.

Oops, you're correct, sorry about that!

The rpath would be pointing to the compiler installation location. This means that only modifying libraries inside the compilation installation location would modify the application behavior.

Maybe that's sufficient mitigation for this to not be a security concern (I don't know)? We'd really need a security expert to weigh in on this for me to feel confident in the change, given my fairly light knowledge of rpath (as a predominately Windows developer, I've always found rpath behavior to be confusing).

[npmiller]

Note that with https://github.com/intel/llvm/pull/6658

libsycl.so is now able to find the plugin even without LD_LIBRARY_PATH set, which solves the issue raised earlier, so now this patch would work better.

[pvchupin]

@cperkinsintel, can you review please?

[wphuhn-intel]

Making my comments here since this topic has been revived again in my inbox:

• My recollection from when this was brought to my attention back in October is that we’re setting an absolute RPATH to the compiler install. The main security concern with absolute RPATH is developers accidentally setting RPATH to, say, a file in the /tmp folder, which is often world-writable and thus could be injected by an attacker, and the developer would be unaware because developers usually don't check RPATHs in their executables.
• It is unlikely that the compiler install directory is world-writable, and if it were, an attacker could have a field day with any other number of runtimes. However, this is not impossible either.
• There’s still the concern about portability, i.e. if I move an executable from one machine to another, could the properly-privileged location of the compiler install on the old machine now be a world-writable directory on the new machine.
• Even if we believe that using absolute RPATH is an acceptable security/usability compromise, it still makes me very uneasy: as @AaronBallman pointed out earlier, there are standard security checkers which flag on RPATH usage so we could get flooded with security reports, and distros like Debian discourage RPATH usage (https://wiki.debian.org/RpathIssue, supposedly Fedora does too but I couldn't find a definitive link for that), so we could have surprised developers if we make this the default.
• Thinking as an ex-GPGPU developer, I don’t understand why we believe setting LD_LIBRARY_PATH is too difficult for the developers to use. Other major offloading frameworks require this be done as part of their standard workflow; it’s something we can expect our customers know how to do.

I don't have the authority to rubber stamp PRs as "Officially Secure" or "Officially Vulnerable" (nor do I want it), but I personally question why we feel -fsycl-implicit-rpath should be the default, I can't help but shake the feeling this is an XY problem.

[mdtoguchi]

As an aside, it looks like -fopenmp-implicit-rpath support has been removed/reverted

[npmiller]

After reading through everything again, given all the legitimate concerns and the fact that the equivalent OpenMP feature has now also been reverted upstream (https://reviews.llvm.org/D118493#4178250) after similar discussions, I think it only makes sense to close this pull request now.

Thanks everyone for all the great input and discussion!