| Message ID | 20210220121610.3982-3-bastiangermann@fishpost.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | debian: Integrate Debian/Ubuntu changes | expand |
On Sat, Feb 20, 2021 at 01:16:07PM +0100, Bastian Germann wrote: > This is a change introduced in 5.6.0-1ubuntu3. > > Reported-by: Dimitri John Ledkov <xnox@ubuntu.com> > Signed-off-by: Bastian Germann <bastiangermann@fishpost.de> > --- > debian/changelog | 1 + > debian/rules | 8 +++++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/debian/changelog b/debian/changelog > index 8320a2e8..c77f04ab 100644 > --- a/debian/changelog > +++ b/debian/changelog > @@ -2,6 +2,7 @@ xfsprogs (5.11.0-rc0-1) experimental; urgency=medium > > [ Dimitri John Ledkov ] > * Drop trying to create upstream distribution > + * Enable CET on amd64 > > -- Bastian Germann <bastiangermann@fishpost.de> Sat, 20 Feb 2021 11:57:31 +0100 > > diff --git a/debian/rules b/debian/rules > index 8a3345b6..dd093f2c 100755 > --- a/debian/rules > +++ b/debian/rules > @@ -23,8 +23,14 @@ pkgdev = DIST_ROOT=`pwd`/$(dirdev); export DIST_ROOT; > pkgdi = DIST_ROOT=`pwd`/$(dirdi); export DIST_ROOT; > stdenv = @GZIP=-q; export GZIP; > > +ifeq ($(target),amd64) > +export DEB_CFLAGS_MAINT_APPEND=-fcf-protection > +export DEB_LDFLAGS_MAINT_APPEND=-fcf-protection > +endif > +include /usr/share/dpkg/default.mk > + > options = export DEBUG=-DNDEBUG DISTRIBUTION=debian \ > - INSTALL_USER=root INSTALL_GROUP=root \ > + INSTALL_USER=root INSTALL_GROUP=root LDFLAGS='$(LDFLAGS)' \ > LOCAL_CONFIGURE_OPTIONS="--enable-editline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ; > diopts = $(options) \ > export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ; No. This is not the way to turn on build wide compiler/linker options/protections. IOWs, if you want to turn on control flow protections to make ROP exploits harder (why that actually matters for xfsprogs is beyond me), then it you need to add a configure option similar to --enable-lto. Then it can actually be enabled and used by other distros, not just Ubuntu, and it will also ensure that builds will fail at configure time if the compiler/linker does not support this functionality. Cheers, Dave.
The patch in question is specific to Ubuntu and was not submitted by me to neither Debian or Upstream. Indeed, this is very distro specific, because of all the other things that we turn on by default in our toolchain, dpkg build flags, and all other packages. This patch if taken at face value, will not enable CET. And will make the package start failing to build from source, when using older toolchains that don't support said flag. It should not go upstream nor into debian. NACK On Sun, Feb 21, 2021 at 3:59 AM Dave Chinner <david@fromorbit.com> wrote: > > On Sat, Feb 20, 2021 at 01:16:07PM +0100, Bastian Germann wrote: > > This is a change introduced in 5.6.0-1ubuntu3. > > > > Reported-by: Dimitri John Ledkov <xnox@ubuntu.com> > > Signed-off-by: Bastian Germann <bastiangermann@fishpost.de> > > --- > > debian/changelog | 1 + > > debian/rules | 8 +++++++- > > 2 files changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/debian/changelog b/debian/changelog > > index 8320a2e8..c77f04ab 100644 > > --- a/debian/changelog > > +++ b/debian/changelog > > @@ -2,6 +2,7 @@ xfsprogs (5.11.0-rc0-1) experimental; urgency=medium > > > > [ Dimitri John Ledkov ] > > * Drop trying to create upstream distribution > > + * Enable CET on amd64 > > > > -- Bastian Germann <bastiangermann@fishpost.de> Sat, 20 Feb 2021 11:57:31 +0100 > > > > diff --git a/debian/rules b/debian/rules > > index 8a3345b6..dd093f2c 100755 > > --- a/debian/rules > > +++ b/debian/rules > > @@ -23,8 +23,14 @@ pkgdev = DIST_ROOT=`pwd`/$(dirdev); export DIST_ROOT; > > pkgdi = DIST_ROOT=`pwd`/$(dirdi); export DIST_ROOT; > > stdenv = @GZIP=-q; export GZIP; > > > > +ifeq ($(target),amd64) > > +export DEB_CFLAGS_MAINT_APPEND=-fcf-protection > > +export DEB_LDFLAGS_MAINT_APPEND=-fcf-protection > > +endif > > +include /usr/share/dpkg/default.mk > > + > > options = export DEBUG=-DNDEBUG DISTRIBUTION=debian \ > > - INSTALL_USER=root INSTALL_GROUP=root \ > > + INSTALL_USER=root INSTALL_GROUP=root LDFLAGS='$(LDFLAGS)' \ > > LOCAL_CONFIGURE_OPTIONS="--enable-editline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ; > > diopts = $(options) \ > > export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ; > > No. This is not the way to turn on build wide compiler/linker > options/protections. > > IOWs, if you want to turn on control flow protections to make ROP > exploits harder (why that actually matters for xfsprogs is beyond > me), then it you need to add a configure option similar to > --enable-lto. Then it can actually be enabled and used by other > distros, not just Ubuntu, and it will also ensure that builds will > fail at configure time if the compiler/linker does not support this > functionality. > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Sun, Feb 21, 2021 at 04:02:55AM +0000, Dimitri John Ledkov wrote: > The patch in question is specific to Ubuntu and was not submitted by > me to neither Debian or Upstream. > > Indeed, this is very distro specific, because of all the other things > that we turn on by default in our toolchain, dpkg build flags, and all > other packages. > > This patch if taken at face value, will not enable CET. And will make > the package start failing to build from source, when using older > toolchains that don't support said flag. Yes, that is exactly what I said when pointing out how to *support this properly* so it doesn't break builds in environments that do not support such functionality. Having it as a configure option allows the configure script to -test whether the toolchain supports it- and then either fail (enable=yes) or not use it (enable=probe) and continue the build without it. > It should not go upstream nor into debian. There is no reason it cannot be implemented as a build option in the upstream package. Then you can get rid of all your nasty hacks and simply add --enable-cf-protections to your distro's configure options. And other distros that also support all this functionality can use it to. Please play nice with others and do things the right way instead of making silly claims about how "nobody else can use this" when it's clear that they can if they also tick all the necessary boxes. Cheers, Dave.
On Sun, Feb 21, 2021 at 4:28 AM Dave Chinner <david@fromorbit.com> wrote: > > On Sun, Feb 21, 2021 at 04:02:55AM +0000, Dimitri John Ledkov wrote: > > The patch in question is specific to Ubuntu and was not submitted by > > me to neither Debian or Upstream. > > > > Indeed, this is very distro specific, because of all the other things > > that we turn on by default in our toolchain, dpkg build flags, and all > > other packages. > > > > This patch if taken at face value, will not enable CET. And will make > > the package start failing to build from source, when using older > > toolchains that don't support said flag. > > Yes, that is exactly what I said when pointing out how to *support > this properly* so it doesn't break builds in environments that do > not support such functionality. > > Having it as a configure option allows the configure script to -test > whether the toolchain supports it- and then either fail (enable=yes) > or not use it (enable=probe) and continue the build without it. > > > It should not go upstream nor into debian. > > There is no reason it cannot be implemented as a build option in the > upstream package. Then you can get rid of all your nasty hacks and > simply add --enable-cf-protections to your distro's configure > options. > > And other distros that also support all this functionality can use > it to. Please play nice with others and do things the right way > instead of making silly claims about how "nobody else can use this" > when it's clear that they can if they also tick all the necessary > boxes. debian will probably will not want --enable-cf-protections as a configure option, and will enable CET via dpkg-buildflags as a hardening option, which will then be turned on by default for relevant architectures and series as of when debian kernel starts to support it. as that way, debian will be able to affect that change across the whole distro. once CET is actually merged into kernel, I do not expect configure options or debian/rules changes to enable CET. At most `export DEB_BUILD_MAINT_OPTIONS=hardening` should be enough in debian/rules.
On Sun, Feb 21, 2021 at 04:32:46AM +0000, Dimitri John Ledkov wrote: > On Sun, Feb 21, 2021 at 4:28 AM Dave Chinner <david@fromorbit.com> wrote: > > > > On Sun, Feb 21, 2021 at 04:02:55AM +0000, Dimitri John Ledkov wrote: > > > The patch in question is specific to Ubuntu and was not submitted by > > > me to neither Debian or Upstream. > > > > > > Indeed, this is very distro specific, because of all the other things > > > that we turn on by default in our toolchain, dpkg build flags, and all > > > other packages. > > > > > > This patch if taken at face value, will not enable CET. And will make > > > the package start failing to build from source, when using older > > > toolchains that don't support said flag. > > > > Yes, that is exactly what I said when pointing out how to *support > > this properly* so it doesn't break builds in environments that do > > not support such functionality. > > > > Having it as a configure option allows the configure script to -test > > whether the toolchain supports it- and then either fail (enable=yes) > > or not use it (enable=probe) and continue the build without it. > > > > > It should not go upstream nor into debian. > > > > There is no reason it cannot be implemented as a build option in the > > upstream package. Then you can get rid of all your nasty hacks and > > simply add --enable-cf-protections to your distro's configure > > options. > > > > And other distros that also support all this functionality can use > > it to. Please play nice with others and do things the right way > > instead of making silly claims about how "nobody else can use this" > > when it's clear that they can if they also tick all the necessary > > boxes. > > debian will probably will not want --enable-cf-protections as a > configure option, and will enable CET via dpkg-buildflags as a > hardening option, which will then be turned on by default for relevant > architectures and series as of when debian kernel starts to support > it. > > as that way, debian will be able to affect that change across the whole distro. Except now you've got the problem that dpkg-buildflags is not used by various packages such as xfsprogs, so the flags it sets up, even with overrides like DEB_BUILD_MAINT_OPTIONS are completely ignored by the debian package build because debian/rules is not set up to source the buildflags at all. e.g. if you run 'make Q= deb' you won't see any of the compiler or linker options emitted by dpkg-buildflags being used during the debian package build. You will, OTOH, see it emit stuff like LTO options because those get turned on via configure. > once CET is actually merged into kernel, I do not expect configure > options or debian/rules changes to enable CET. At most `export > DEB_BUILD_MAINT_OPTIONS=hardening` should be enough in debian/rules. The default dpkg-buildflags output already includes "hardening" options, and as I said above, they aren't actually included in the build rules. Hence what you suggest just won't work like you think it should. Again, if you want build option stuff to work correctly, don't rely on distro specific magic to turn stuff on because it just doesn't work for everyone. Use configure options to enable, probe and detect support and enable the feature in the build, then upstream and other distros can actually build with it and test it outside the magical unicorn distro build environment you are running in... Cheers, Dave.
diff --git a/debian/changelog b/debian/changelog index 8320a2e8..c77f04ab 100644 --- a/debian/changelog +++ b/debian/changelog @@ -2,6 +2,7 @@ xfsprogs (5.11.0-rc0-1) experimental; urgency=medium [ Dimitri John Ledkov ] * Drop trying to create upstream distribution + * Enable CET on amd64 -- Bastian Germann <bastiangermann@fishpost.de> Sat, 20 Feb 2021 11:57:31 +0100 diff --git a/debian/rules b/debian/rules index 8a3345b6..dd093f2c 100755 --- a/debian/rules +++ b/debian/rules @@ -23,8 +23,14 @@ pkgdev = DIST_ROOT=`pwd`/$(dirdev); export DIST_ROOT; pkgdi = DIST_ROOT=`pwd`/$(dirdi); export DIST_ROOT; stdenv = @GZIP=-q; export GZIP; +ifeq ($(target),amd64) +export DEB_CFLAGS_MAINT_APPEND=-fcf-protection +export DEB_LDFLAGS_MAINT_APPEND=-fcf-protection +endif +include /usr/share/dpkg/default.mk + options = export DEBUG=-DNDEBUG DISTRIBUTION=debian \ - INSTALL_USER=root INSTALL_GROUP=root \ + INSTALL_USER=root INSTALL_GROUP=root LDFLAGS='$(LDFLAGS)' \ LOCAL_CONFIGURE_OPTIONS="--enable-editline=yes --enable-blkid=yes --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ; diopts = $(options) \ export OPTIMIZER=-Os LOCAL_CONFIGURE_OPTIONS="--enable-gettext=no --disable-ubsan --disable-addrsan --disable-threadsan --enable-lto" ;
This is a change introduced in 5.6.0-1ubuntu3. Reported-by: Dimitri John Ledkov <xnox@ubuntu.com> Signed-off-by: Bastian Germann <bastiangermann@fishpost.de> --- debian/changelog | 1 + debian/rules | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-)