[CMake] Set runpath of libsycl.so

[RossBrunton]

libsycl.so depends on libur_loader.so, however it doesn't have an rpath
set to find it. This fixes that.

[RossBrunton]

Problem that this patch fixes:

mkdir sycl;
cd sycl;
wget https://github.com/intel/llvm/releases/download/nightly-2024-10-23/sycl_linux.tar.gz;
tar -xzf sycl_linux.tar.gz;
echo "int main() {}" > testfile.cpp;
./bin/clang testfile.cpp -fsycl -o /dev/null;

This fails because the rpath of libsycl isn't set in the installed package, so it can't find libur_loader, which causes linking to fail since it can't find a bunch of symbols.

See this CI run as well: https://github.com/oneapi-src/unified-runtime/actions/runs/11497879935/job/32002518200?pr=2225

If a better solution is to just pass -lur_loader whenever we pass -lsycl, let me know. I'm not sure which option is best.

[steffenlarsen]

If a better solution is to just pass -lur_loader whenever we pass -lsycl, let me know. I'm not sure which option is best.

I take it this would be part of the driver's job? Tag @mdtoguchi for comment.

[mdtoguchi]

If a better solution is to just pass -lur_loader whenever we pass -lsycl, let me know. I'm not sure which option is best.

I take it this would be part of the driver's job? Tag @mdtoguchi for comment.

As the dependency on libur_loader is from the library, I would rather keep it there than have the driver pull it in. If there is a dependency of compiled source on libur_loader we can have the driver do it.

[bader]

Similar change has been rejected in the past due to security concerns.
@RossBrunton, please, read the discussion in #6306 comments. Could you confirm that this change doesn't have similar problems?
@wphuhn-intel, FYI.

@intel/llvm-reviewers-runtime, please, review carefully patches adding rpath to the SYCL runtime library and be aware of associated security risks.

[RossBrunton]

Thanks for the link, I think this is a different case to -fsycl-implicit-rpath since we aren't "infecting" created binaries with an arbitrary path that downstream customers could control.

The only ways I could see an attacker take advantage of this change is under the following conditions:

• At the time the user builds their SYCL program, the directory containing libsycl.so is world writeable and an attacker replaces/inserts a new version of ur_loader.so.
• The user installs a version of libsycl.so into a world writeable directory (like /tmp), uses that when building their SYCL program, and attacker sneaks in a version of ur_loader.so into the same folder.

I don't think these are likely. And if they were, bin/clang has the same issue, since it uses an rpath to find libsycl.so.

Of course, happy to have more eyes on this to see if there's anything I've missed or there's a better way of solving this issue.

[RossBrunton]

@bader @wphuhn-intel Can I get this looked at? As it stands at the moment, the release packages are broken.

[wphuhn-intel]

My comments:

• bin/clang is using RUNPATH, not RPATH:

> readelf -d ./clang | grep PATH
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../../lib]

• This is likely due to CMake deciding to use RUNPATH on its own instead of RPATH, ignoring its own option naming scheme ( https://discourse.cmake.org/t/setting-rpath-and-not-runpath-for-executable/10011 ). My guess is this patch will similarly enable RUNPATH instead.
• RUNPATH is generally considered better than RPATH since it respects LD_LIBRARY_PATH, but this could end up being a bug in-and-of-itself if libsycl.so ends up picking up the wrong ur_loader.so from LD_LIBRARY_PATH instead of the one it intended to find in $ORIGIN.
• I could see issues with customers redistributing libsycl.so to their customers but not knowing that they need to redistribute ur_loader.so as well. This would need to be documented at bare minimum.
• There is still the issue of security checkers not liking RUNPATH, as well as distribution packagers.
• There's still my befuddlement at why LD_LIBRARY_PATH isn't sufficient, but I have my suspicions.
• I still don't have the authority to rubber stamp PRs as "Officially Secure" or "Officially Vulnerable", and I have even less of a desire for it than I had two years ago.

My general thought is that this is definitely less egregious than the original -fsycl-implicit-rpath proposal since we're owning the security issues ourselves here, however I'm still a little worried about the library redistribution issue, and the whole affair has a code smell to it.

[RossBrunton]

@wphuhn-intel
Using LD_LIBRARY_PATH does work. For example, the following compiles fine:

LD_LIBRARY_PATH=/tmp/sycl/lib ./bin/clang testfile.cpp -fsycl -o /dev/null;

(assuming the package is extracted in /tmp/sycl, of course). However, I don't think it's reasonable to expect users to set LD_LIBRARY_PATH just to invoke the compiler.

With regards to packaging, I think it should be fine for them to strip the runpath if they install libur_loader.so to a standard location (e.g. /usr/lib). Even if we want to install into /usr/lib/oneapi or similar, I think it's acceptable for debian at least ( https://wiki.debian.org/RpathIssue ) to use runpath there.

Thanks for pointing out the rpath vs runpath distinction; I think I just conflated the two. When I get the chance I will check which of the two is being set and update the commit message/comments accordingly.

[aelovikov-intel]

It's important to support scenario when we compile an app using one clang, but then use libsycl.so from LD_LIBRARY_PATH of another build (for debugging purposes). Can you please add a test that verifies that behavior somehow?

[RossBrunton]

@aelovikov-intel
I'm not sure this patch makes a difference for that use case - LD_LIBRARY_PATH will contain a path to a directory that contains both libsycl and libur_loader, and LD_LIBRARY_PATH always has priority over the runpath.

Checking that a version of libsycl can run binaries created by an older/newer version of DPCPP feels like it should be covered with ABI testing, which I think is out of scope of this change.

[aelovikov-intel]

@aelovikov-intel I'm not sure this patch makes a difference for that use case - LD_LIBRARY_PATH will contain a path to a directory that contains both libsycl and libur_loader, and LD_LIBRARY_PATH always has priority over the runpath.

Checking that a version of libsycl can run binaries created by an older/newer version of DPCPP feels like it should be covered with ABI testing, which I think is out of scope of this change.

From what I read in this PR, it's true for RUNPATH but not RPATH (I might have misunderstood), and even if that was true, it wasn't immediately obvious for all the participants here. As such, we need to have a test in tree that will pass after this PR gets merged but would fail if one was to use RPATH over RUNPATH.

[RossBrunton]

$ objdump -x libsycl.so |grep 'R.*PATH'
RUNPATH              $ORIGIN

Cmake actually sets the runpath, not the rpath, I've updated the commit message and comment to say so.

I'll have a look at testing this tomorrow.