[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.

[AlexeySachkov]

• 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 is already happening and I don't see how presence of RUNPATH affects this

There's still my befuddlement at why LD_LIBRARY_PATH isn't sufficient, but I have my suspicions.

It depends on the way of how SYCL RT is redistributed. For example, it could be bundled alongside a client application aimed for end users: that category of users does not want to tweak LD_LIBRARY_PATH to run their app, the app should just work after installation.

There is still the issue of security checkers not liking RUNPATH, as well as distribution packagers.

I'm not sure what to do with security checkers, but at least we can make RUNPATH optional to satisfy distribution packagers

[wphuhn-intel]

@AlexeySachkov IIRC we subsequently found other instances of RUNPATH being used, and we haven't heard any complaints, so that could be overcaution on my end.

I would say that as long as we've confirmed that RPATH isn't being used, we should put this to rest.

[RossBrunton]

Binary information about "installed" libsycl.so before this patch:

$ ldd libsycl.so
linux-vdso.so.1
libur_loader.so.0 => not found
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1
libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6
/lib64/ld-linux-x86-64.so.2

$ readelf -d libsycl.so
Dynamic section at offset 0x401b60 contains 31 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libur_loader.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libstdc++.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [ld-linux-x86-64.so.2]
 0x000000000000000e (SONAME)             Library soname: [libsycl.so.8]
 0x000000000000000c (INIT)               0x77000
 0x000000000000000d (FINI)               0x3576ec
 0x0000000000000019 (INIT_ARRAY)         0x3ff690
 0x000000000000001b (INIT_ARRAYSZ)       320 (bytes)
 0x000000000000001a (FINI_ARRAY)         0x3ff7d0
 0x000000000000001c (FINI_ARRAYSZ)       16 (bytes)
 0x000000006ffffef5 (GNU_HASH)           0x328
 0x0000000000000005 (STRTAB)             0x245d8
 0x0000000000000006 (SYMTAB)             0x9398
 0x000000000000000a (STRSZ)              270707 (bytes)
 0x000000000000000b (SYMENT)             24 (bytes)
 0x0000000000000003 (PLTGOT)             0x402fe8
 0x0000000000000002 (PLTRELSZ)           15816 (bytes)
 0x0000000000000014 (PLTREL)             RELA
 0x0000000000000017 (JMPREL)             0x72838
 0x0000000000000007 (RELA)               0x68e60
 0x0000000000000008 (RELASZ)             39384 (bytes)
 0x0000000000000009 (RELAENT)            24 (bytes)
 0x000000006ffffffb (FLAGS_1)            Flags: NODELETE
 0x000000006ffffffe (VERNEED)            0x68b80
 0x000000006fffffff (VERNEEDNUM)         6
 0x000000006ffffff0 (VERSYM)             0x6674c
 0x000000006ffffff9 (RELACOUNT)          1300
 0x0000000000000000 (NULL)               0x0

And after this patch:

$ ldd libsycl.so
linux-vdso.so.1 (0x00007ffdc7f7d000)
libur_loader.so.0 => /home/ross/build/dpcpp.release/install/lib/./libur_loader.so.0
libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1
libstdc++.so.6 => /lib/x86_64-linux-gnu/libstdc++.so.6
libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6
libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6
/lib64/ld-linux-x86-64.so.2

$ readelf -d libsycl.so
Dynamic section at offset 0x401b60 contains 32 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libur_loader.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libstdc++.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [ld-linux-x86-64.so.2]
 0x000000000000000e (SONAME)             Library soname: [libsycl.so.8]
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN]
 0x000000000000000c (INIT)               0x77000
 0x000000000000000d (FINI)               0x3576ec
 0x0000000000000019 (INIT_ARRAY)         0x3ff690
 0x000000000000001b (INIT_ARRAYSZ)       320 (bytes)
 0x000000000000001a (FINI_ARRAY)         0x3ff7d0
 0x000000000000001c (FINI_ARRAYSZ)       16 (bytes)
 0x000000006ffffef5 (GNU_HASH)           0x328
 0x0000000000000005 (STRTAB)             0x245d8
 0x0000000000000006 (SYMTAB)             0x9398
 0x000000000000000a (STRSZ)              270707 (bytes)
 0x000000000000000b (SYMENT)             24 (bytes)
 0x0000000000000003 (PLTGOT)             0x402fe8
 0x0000000000000002 (PLTRELSZ)           15816 (bytes)
 0x0000000000000014 (PLTREL)             RELA
 0x0000000000000017 (JMPREL)             0x72838
 0x0000000000000007 (RELA)               0x68e60
 0x0000000000000008 (RELASZ)             39384 (bytes)
 0x0000000000000009 (RELAENT)            24 (bytes)
 0x000000006ffffffb (FLAGS_1)            Flags: NODELETE
 0x000000006ffffffe (VERNEED)            0x68b80
 0x000000006fffffff (VERNEEDNUM)         6
 0x000000006ffffff0 (VERSYM)             0x6674c
 0x000000006ffffff9 (RELACOUNT)          1300
 0x0000000000000000 (NULL)               0x0

This change does indeed set RUNPATH rather than RPATH.

In regards to testing this, I'm not sure the best way to actually test it. It requires an "install" of dpcpp rather than just a "build", which I don't think is something that our testing infrastructure really uses? Perhaps this makes sense to check for just before the binaries are bundled into the release archive rather than CI?

[RossBrunton]

@intel/llvm-reviewers-runtime I have updated this and am looking at trying to land it again. Could I get some eyes on this to see if the concerns raised earlier have been resolved?

[RossBrunton]

@intel/llvm-reviewers-runtime Can you look at this when you get the chance?

[RossBrunton]

@intel/llvm-reviewers-runtime Anything blocking this?

[steffenlarsen]

Tag @aelovikov-intel to answer whether he agrees that testing this in CI isn't feasible.

[aelovikov-intel]

I'd feel more secure if we'd add a test that performs readelf/ldd commands from above and verifies that it uses RUNPATH and doesn't use RPATH.

[RossBrunton]

@aelovikov-intel

Broadly, speaking the build pipeline works like this:

• configure
• build
• test
• install

Cmake only sets the runpath during the install phase, after we've already run our tests.

$ readelf -d lib/libsycl.so | grep RUNPATH
 0x000000000000001d (RUNPATH)            Library runpath: [/home/ross/build/dpcpp.release/lib:]
$ readelf -d install/lib/libsycl.so | grep RUNPATH
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN]

We can't do these tests as part of the normal sycl-e2e testing, since it'll see the "build" version of libsycl.so rather than the "install" version.

Any testing for runpath would have to be done after the install phase, perhaps as a new phase in the CI pipeline or as a script ran by the packager just before it generates packages for distribution. Such a script could probably also check other libraries and binaries for the same issue.

I don't disagree with such a thing existing, but I think it's out of scope for this change.

[aelovikov-intel]

The one we use in CI is from install:

I do understand that a test would need to work locally with the one from the build/ too, so not sure how to structure it.

[RossBrunton]

@intel/llvm-gatekeepers Please merge.

I'll create a ticket on Intel's internal Jira about looking into verifying that produced binaries don't use RPATH.