| Message ID | 20200227230129.31166-3-william.c.roberts@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] dso: drop hidden_proto and hidden_def | expand |
On Thu, Feb 27, 2020 at 6:01 PM <bill.c.roberts@gmail.com> wrote: > > From: William Roberts <william.c.roberts@intel.com> > > Add -fno-semantic-interposition to CFLAGS. This will restore > the DSO infrastructures protections to insure internal callers > of exported symbols call into libselinux and not something laoding first > in the library list. > > Clang has this enabled by default. > > Signed-off-by: William Roberts <william.c.roberts@intel.com> I'm fine with this but since Nicolas pointed out the option of using -Bsymbolic to the linker as an alternative to hidden_def/hidden_proto in https://github.com/SELinuxProject/selinux/issues/204#issuecomment-591092288 I was wondering how they differ. I guess -Bsymbolic only affects the linker while -fno-semantic-interposition permits the compiler to further optimize the code.
On Fri, Feb 28, 2020 at 7:50 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Feb 27, 2020 at 6:01 PM <bill.c.roberts@gmail.com> wrote: > > > > From: William Roberts <william.c.roberts@intel.com> > > > > Add -fno-semantic-interposition to CFLAGS. This will restore > > the DSO infrastructures protections to insure internal callers > > of exported symbols call into libselinux and not something laoding first > > in the library list. > > > > Clang has this enabled by default. > > > > Signed-off-by: William Roberts <william.c.roberts@intel.com> > > I'm fine with this but since Nicolas pointed out the option of using > -Bsymbolic to > the linker as an alternative to hidden_def/hidden_proto in > https://github.com/SELinuxProject/selinux/issues/204#issuecomment-591092288 > I was wondering how they differ. I guess -Bsymbolic only affects the > linker while -fno-semantic-interposition > permits the compiler to further optimize the code. That's my understanding of the difference as well. -B is only link time, so the compiler can't really optimize the calls, but IIUC the compiler can only optimize whats in the compilation unit, so it can only optimize call sites for calls within the compilation unit. -B also only works for elf builds, so we would conditionally need to modify LDFLAGS based on host type. The compiler option is just a clang/gcc split, which is already there. To go to symbolic we would have to conditionally set that... a tad bit more work :-p
On Fri, Feb 28, 2020 at 2:59 PM William Roberts <bill.c.roberts@gmail.com> wrote: > > On Fri, Feb 28, 2020 at 7:50 AM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > > > On Thu, Feb 27, 2020 at 6:01 PM <bill.c.roberts@gmail.com> wrote: > > > > > > From: William Roberts <william.c.roberts@intel.com> > > > > > > Add -fno-semantic-interposition to CFLAGS. This will restore > > > the DSO infrastructures protections to insure internal callers > > > of exported symbols call into libselinux and not something laoding first > > > in the library list. > > > > > > Clang has this enabled by default. > > > > > > Signed-off-by: William Roberts <william.c.roberts@intel.com> > > > > I'm fine with this but since Nicolas pointed out the option of using > > -Bsymbolic to > > the linker as an alternative to hidden_def/hidden_proto in > > https://github.com/SELinuxProject/selinux/issues/204#issuecomment-591092288 > > I was wondering how they differ. I guess -Bsymbolic only affects the > > linker while -fno-semantic-interposition > > permits the compiler to further optimize the code. > > That's my understanding of the difference as well. -B is only link > time, so the compiler can't > really optimize the calls, but IIUC the compiler can only optimize > whats in the compilation > unit, so it can only optimize call sites for calls within the compilation unit. > > -B also only works for elf builds, so we would conditionally need to > modify LDFLAGS based > on host type. The compiler option is just a clang/gcc split, which is > already there. To go > to symbolic we would have to conditionally set that... a tad bit more work :-p I am not very familiar with -Bsymbolic and did not know about -fno-semantic-interposition before a few days ago. As far as I understand, -Bsymbolic puts an entry of type "SYMBOLIC" in the dynamic section of the .so file, which changes the behavior of the dynamic linker at link time without modifying anything in the build-time linking process. It is only compatible with ELF files and seems quite uncommon. With this in mind, -fno-semantic-interposition seems more powerful and better suited for what we want to achieve (to optimize the compilation). I agree with the approach of this patchset and would have given an Acked-By if there weren't an issue with "-z,relro-Wl" ;) Thanks for working on this! Nicolas
Version 4:
- Fix linker option warnings.
- Move map file to begining of options.
Version 3:
- Add more symbols that should be dropped from the dso:
- map_class;
- map_decision;
- map_perm;
Version 2:
- adds a version to the linker script LIBSELINUX_1.0
- Adds a patch to drop some additional symbols from the dso:
- dir_xattr_list
- myprintf_compat
- unmap_class
- unmap_perm
This four part patch series drops the dso.h and hidden_*
macros.
The old dso.h functionality provided libselinux with both control over
external exported symbols as well as ensuring internal callers call into
libselinux and not a symbol with the same name loaded by the linker
earlier in the library list.
The functionality is replaced by a linker script that requires public
API to explicitly be opt-in. The old method required that internal API
be explicitly annotated, and everything else is public. This should help
make it easier to control libselinux DSO hygene going forward.
The second functionality is replaced by compiler option
-fno-semantic-interposition
Note that clang has this enabled by default, and thus doesn't need it.
See:
- https://stackoverflow.com/questions/35745543/new-option-in-gcc-5-3-fno-semantic-interposition
[PATCH v4 1/4] dso: drop hidden_proto and hidden_def
[PATCH v4 2/4] Makefile: add -fno-semantic-interposition
[PATCH v4 3/4] Makefile: add linker script to minimize exports
[PATCH v4 4/4] libselinux: drop symbols from map
On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: > > Version 4: > - Fix linker option warnings. > - Move map file to begining of options. > > Version 3: > - Add more symbols that should be dropped from the dso: > - map_class; > - map_decision; > - map_perm; > > Version 2: > - adds a version to the linker script LIBSELINUX_1.0 > - Adds a patch to drop some additional symbols from the dso: > - dir_xattr_list > - myprintf_compat > - unmap_class > - unmap_perm > > This four part patch series drops the dso.h and hidden_* > macros. > > The old dso.h functionality provided libselinux with both control over > external exported symbols as well as ensuring internal callers call into > libselinux and not a symbol with the same name loaded by the linker > earlier in the library list. > > The functionality is replaced by a linker script that requires public > API to explicitly be opt-in. The old method required that internal API > be explicitly annotated, and everything else is public. This should help > make it easier to control libselinux DSO hygene going forward. > > The second functionality is replaced by compiler option > -fno-semantic-interposition > > Note that clang has this enabled by default, and thus doesn't need it. > > See: > - https://stackoverflow.com/questions/35745543/new-option-in-gcc-5-3-fno-semantic-interposition > > [PATCH v4 1/4] dso: drop hidden_proto and hidden_def > [PATCH v4 2/4] Makefile: add -fno-semantic-interposition > [PATCH v4 3/4] Makefile: add linker script to minimize exports > [PATCH v4 4/4] libselinux: drop symbols from map This looks fine to me but I'd like at least one of the distro maintainers to ack it (especially the last one).
On Tue, Mar 3, 2020 at 7:57 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: > > > > Version 4: > > - Fix linker option warnings. > > - Move map file to begining of options. > > > > Version 3: > > - Add more symbols that should be dropped from the dso: > > - map_class; > > - map_decision; > > - map_perm; > > > > Version 2: > > - adds a version to the linker script LIBSELINUX_1.0 > > - Adds a patch to drop some additional symbols from the dso: > > - dir_xattr_list > > - myprintf_compat > > - unmap_class > > - unmap_perm > > > > This four part patch series drops the dso.h and hidden_* > > macros. > > > > The old dso.h functionality provided libselinux with both control over > > external exported symbols as well as ensuring internal callers call into > > libselinux and not a symbol with the same name loaded by the linker > > earlier in the library list. > > > > The functionality is replaced by a linker script that requires public > > API to explicitly be opt-in. The old method required that internal API > > be explicitly annotated, and everything else is public. This should help > > make it easier to control libselinux DSO hygene going forward. > > > > The second functionality is replaced by compiler option > > -fno-semantic-interposition > > > > Note that clang has this enabled by default, and thus doesn't need it. > > > > See: > > - https://stackoverflow.com/questions/35745543/new-option-in-gcc-5-3-fno-semantic-interposition > > > > [PATCH v4 1/4] dso: drop hidden_proto and hidden_def > > [PATCH v4 2/4] Makefile: add -fno-semantic-interposition > > [PATCH v4 3/4] Makefile: add linker script to minimize exports > > [PATCH v4 4/4] libselinux: drop symbols from map > > This looks fine to me but I'd like at least one of the distro > maintainers to ack it (especially the last one). FWIW, I scanned all Fedora (32) packages that Require: libselinux using this script and it seems that nothing is using the symbols mentioned in patch 4/4 on Fedora: https://gitlab.com/omos/selinux-misc/-/blob/master/scan_imports.sh BTW, the same dso.h infrastructure is used also in libsepol and libsemanage - are there plans to do the same thing for those two? -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
Stephen Smalley <stephen.smalley.work@gmail.com> writes: > On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: >> >> Version 4: >> - Fix linker option warnings. >> - Move map file to begining of options. >> >> Version 3: >> - Add more symbols that should be dropped from the dso: >> - map_class; >> - map_decision; >> - map_perm; >> >> Version 2: >> - adds a version to the linker script LIBSELINUX_1.0 >> - Adds a patch to drop some additional symbols from the dso: >> - dir_xattr_list >> - myprintf_compat >> - unmap_class >> - unmap_perm >> >> This four part patch series drops the dso.h and hidden_* >> macros. >> >> The old dso.h functionality provided libselinux with both control over >> external exported symbols as well as ensuring internal callers call into >> libselinux and not a symbol with the same name loaded by the linker >> earlier in the library list. >> >> The functionality is replaced by a linker script that requires public >> API to explicitly be opt-in. The old method required that internal API >> be explicitly annotated, and everything else is public. This should help >> make it easier to control libselinux DSO hygene going forward. >> >> The second functionality is replaced by compiler option >> -fno-semantic-interposition >> >> Note that clang has this enabled by default, and thus doesn't need it. >> >> See: >> - https://stackoverflow.com/questions/35745543/new-option-in-gcc-5-3-fno-semantic-interposition >> >> [PATCH v4 1/4] dso: drop hidden_proto and hidden_def >> [PATCH v4 2/4] Makefile: add -fno-semantic-interposition >> [PATCH v4 3/4] Makefile: add linker script to minimize exports >> [PATCH v4 4/4] libselinux: drop symbols from map > > This looks fine to me but I'd like at least one of the distro > maintainers to ack it (especially the last one). On it. I'll provide a feedback later today or tomorrow.
<snip> > > This looks fine to me but I'd like at least one of the distro > > maintainers to ack it (especially the last one). > > FWIW, I scanned all Fedora (32) packages that Require: libselinux > using this script and it seems that nothing is using the symbols > mentioned in patch 4/4 on Fedora: > > https://gitlab.com/omos/selinux-misc/-/blob/master/scan_imports.sh > > BTW, the same dso.h infrastructure is used also in libsepol and > libsemanage - are there plans to do the same thing for those two? Yes I can queue that up. I'd like to get through some of the other updates first.
Stephen Smalley <stephen.smalley.work@gmail.com> writes: > On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: >> >> Version 4: >> - Fix linker option warnings. >> - Move map file to begining of options. >> >> Version 3: >> - Add more symbols that should be dropped from the dso: >> - map_class; >> - map_decision; >> - map_perm; >> >> Version 2: >> - adds a version to the linker script LIBSELINUX_1.0 >> - Adds a patch to drop some additional symbols from the dso: >> - dir_xattr_list >> - myprintf_compat >> - unmap_class >> - unmap_perm >> >> This four part patch series drops the dso.h and hidden_* >> macros. >> >> The old dso.h functionality provided libselinux with both control over >> external exported symbols as well as ensuring internal callers call into >> libselinux and not a symbol with the same name loaded by the linker >> earlier in the library list. >> >> The functionality is replaced by a linker script that requires public >> API to explicitly be opt-in. The old method required that internal API >> be explicitly annotated, and everything else is public. This should help >> make it easier to control libselinux DSO hygene going forward. >> >> The second functionality is replaced by compiler option >> -fno-semantic-interposition >> >> Note that clang has this enabled by default, and thus doesn't need it. >> >> See: >> - https://stackoverflow.com/questions/35745543/new-option-in-gcc-5-3-fno-semantic-interposition >> >> [PATCH v4 1/4] dso: drop hidden_proto and hidden_def >> [PATCH v4 2/4] Makefile: add -fno-semantic-interposition >> [PATCH v4 3/4] Makefile: add linker script to minimize exports >> [PATCH v4 4/4] libselinux: drop symbols from map > > This looks fine to me but I'd like at least one of the distro > maintainers to ack it (especially the last one). Acked-by: Petr Lautrbach <plautrba@redhat.com>
On Thu, Mar 5, 2020 at 6:42 AM Petr Lautrbach <plautrba@redhat.com> wrote: > > > Stephen Smalley <stephen.smalley.work@gmail.com> writes: > > > On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: > >> > >> Version 4: > >> - Fix linker option warnings. > >> - Move map file to begining of options. > >> > >> Version 3: > >> - Add more symbols that should be dropped from the dso: > >> - map_class; > >> - map_decision; > >> - map_perm; > >> > >> Version 2: > >> - adds a version to the linker script LIBSELINUX_1.0 > >> - Adds a patch to drop some additional symbols from the dso: > >> - dir_xattr_list > >> - myprintf_compat > >> - unmap_class > >> - unmap_perm > >> > >> This four part patch series drops the dso.h and hidden_* > >> macros. > >> > >> The old dso.h functionality provided libselinux with both control over > >> external exported symbols as well as ensuring internal callers call into > >> libselinux and not a symbol with the same name loaded by the linker > >> earlier in the library list. > >> > >> The functionality is replaced by a linker script that requires public > >> API to explicitly be opt-in. The old method required that internal API > >> be explicitly annotated, and everything else is public. This should help > >> make it easier to control libselinux DSO hygene going forward. > >> > >> The second functionality is replaced by compiler option > >> -fno-semantic-interposition > >> > >> Note that clang has this enabled by default, and thus doesn't need it. > >> > >> See: > >> - https://stackoverflow.com/questions/35745543/new-option-in-gcc-5-3-fno-semantic-interposition > >> > >> [PATCH v4 1/4] dso: drop hidden_proto and hidden_def > >> [PATCH v4 2/4] Makefile: add -fno-semantic-interposition > >> [PATCH v4 3/4] Makefile: add linker script to minimize exports > >> [PATCH v4 4/4] libselinux: drop symbols from map > > > > This looks fine to me but I'd like at least one of the distro > > maintainers to ack it (especially the last one). > > Acked-by: Petr Lautrbach <plautrba@redhat.com> > Thanks staged: https://github.com/SELinuxProject/selinux/pull/205
On Thu, Mar 5, 2020 at 10:12 AM William Roberts <bill.c.roberts@gmail.com> wrote: > > On Thu, Mar 5, 2020 at 6:42 AM Petr Lautrbach <plautrba@redhat.com> wrote: > > > > > > Stephen Smalley <stephen.smalley.work@gmail.com> writes: > > > > > On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: > > >> > > >> Version 4: > > >> - Fix linker option warnings. > > >> - Move map file to begining of options. > > >> > > >> Version 3: > > >> - Add more symbols that should be dropped from the dso: > > >> - map_class; > > >> - map_decision; > > >> - map_perm; > > >> > > >> Version 2: > > >> - adds a version to the linker script LIBSELINUX_1.0 > > >> - Adds a patch to drop some additional symbols from the dso: > > >> - dir_xattr_list > > >> - myprintf_compat > > >> - unmap_class > > >> - unmap_perm > > >> > > >> This four part patch series drops the dso.h and hidden_* > > >> macros. > > >> > > >> The old dso.h functionality provided libselinux with both control over > > >> external exported symbols as well as ensuring internal callers call into > > >> libselinux and not a symbol with the same name loaded by the linker > > >> earlier in the library list. > > >> > > >> The functionality is replaced by a linker script that requires public > > >> API to explicitly be opt-in. The old method required that internal API > > >> be explicitly annotated, and everything else is public. This should help > > >> make it easier to control libselinux DSO hygene going forward. > > >> > > >> The second functionality is replaced by compiler option > > >> -fno-semantic-interposition > > >> > > >> Note that clang has this enabled by default, and thus doesn't need it. > > >> > > >> See: > > >> - https://stackoverflow.com/questions/35745543/new-option-in-gcc-5-3-fno-semantic-interposition > > >> > > >> [PATCH v4 1/4] dso: drop hidden_proto and hidden_def > > >> [PATCH v4 2/4] Makefile: add -fno-semantic-interposition > > >> [PATCH v4 3/4] Makefile: add linker script to minimize exports > > >> [PATCH v4 4/4] libselinux: drop symbols from map > > > > > > This looks fine to me but I'd like at least one of the distro > > > maintainers to ack it (especially the last one). > > > > Acked-by: Petr Lautrbach <plautrba@redhat.com> > > > > Thanks staged: > https://github.com/SELinuxProject/selinux/pull/205 merged. Since folks have interest in doing the same for libsepol and libsemanage, im going to roll those before updating the attribute deprecated patches. Thanks, Bill
On Thu, Mar 5, 2020 at 2:09 PM William Roberts <bill.c.roberts@gmail.com> wrote: > > On Thu, Mar 5, 2020 at 10:12 AM William Roberts > <bill.c.roberts@gmail.com> wrote: > > > > On Thu, Mar 5, 2020 at 6:42 AM Petr Lautrbach <plautrba@redhat.com> wrote: > > > > > > > > > Stephen Smalley <stephen.smalley.work@gmail.com> writes: > > > > > > > On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: > > > >> [PATCH v4 1/4] dso: drop hidden_proto and hidden_def > > > >> [PATCH v4 2/4] Makefile: add -fno-semantic-interposition > > > >> [PATCH v4 3/4] Makefile: add linker script to minimize exports > > > >> [PATCH v4 4/4] libselinux: drop symbols from map > > > > > > > > This looks fine to me but I'd like at least one of the distro > > > > maintainers to ack it (especially the last one). > > > > > > Acked-by: Petr Lautrbach <plautrba@redhat.com> > > > > > > > Thanks staged: > > https://github.com/SELinuxProject/selinux/pull/205 > > merged. I've noticed a change since this was applied; if I build userspace against the updated libselinux, the dynamic loader will print a warning if it is run on a system with the older libselinux, ala: $ /sbin/restorecon -v /etc /sbin/restorecon: /lib64/libselinux.so.1: no version information available (required by /sbin/restorecon) It still works since there was no real change in the ABI but I don't know if this is a concern for distros.
On Wed, Mar 11, 2020 at 1:14 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Mar 5, 2020 at 2:09 PM William Roberts <bill.c.roberts@gmail.com> wrote: > > > > On Thu, Mar 5, 2020 at 10:12 AM William Roberts > > <bill.c.roberts@gmail.com> wrote: > > > > > > On Thu, Mar 5, 2020 at 6:42 AM Petr Lautrbach <plautrba@redhat.com> wrote: > > > > > > > > > > > > Stephen Smalley <stephen.smalley.work@gmail.com> writes: > > > > > > > > > On Mon, Mar 2, 2020 at 11:41 AM <bill.c.roberts@gmail.com> wrote: > > > > >> [PATCH v4 1/4] dso: drop hidden_proto and hidden_def > > > > >> [PATCH v4 2/4] Makefile: add -fno-semantic-interposition > > > > >> [PATCH v4 3/4] Makefile: add linker script to minimize exports > > > > >> [PATCH v4 4/4] libselinux: drop symbols from map > > > > > > > > > > This looks fine to me but I'd like at least one of the distro > > > > > maintainers to ack it (especially the last one). > > > > > > > > Acked-by: Petr Lautrbach <plautrba@redhat.com> > > > > > > > > > > Thanks staged: > > > https://github.com/SELinuxProject/selinux/pull/205 > > > > merged. > > I've noticed a change since this was applied; if I build userspace > against the updated libselinux, the dynamic loader will > print a warning if it is run on a system with the older libselinux, ala: > $ /sbin/restorecon -v /etc > /sbin/restorecon: /lib64/libselinux.so.1: no version information > available (required by /sbin/restorecon) > It still works since there was no real change in the ABI but I don't > know if this is a concern for distros. I replicated this and looked into it, AFAICT its relatively harmless since, as you point out, we're not breaking ABI. But If I understand your comment (paraphrasing), "userspace should all be updated together" from: - https://lore.kernel.org/selinux/CAEjxPJ4UPBWSP0E4pjR+F6uKMZNHK9J7LTL1gVznpwyJh9UWNA@mail.gmail.com/ Then it really shouldn't be an issue. A lot of what I read on the version stuff was written by Urlich, so he might weigh in with more information and perhaps a way to correct this.
diff --git a/libselinux/src/Makefile b/libselinux/src/Makefile index 7f5a5d7418e9..c76110fbc650 100644 --- a/libselinux/src/Makefile +++ b/libselinux/src/Makefile @@ -65,7 +65,7 @@ EXTRA_CFLAGS = -fipa-pure-const -Wlogical-op -Wpacked-bitfield-compat -Wsync-nan -Wcoverage-mismatch -Wcpp -Wformat-contains-nul -Wnormalized=nfc -Wsuggest-attribute=const \ -Wsuggest-attribute=noreturn -Wsuggest-attribute=pure -Wtrampolines -Wjump-misses-init \ -Wno-suggest-attribute=pure -Wno-suggest-attribute=const -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 \ - -Wstrict-overflow=5 + -Wstrict-overflow=5 -fno-semantic-interposition else EXTRA_CFLAGS = -Wunused-command-line-argument endif