diff mbox series

[2/4] debian: Enable CET on amd64

Message ID 20210220121610.3982-3-bastiangermann@fishpost.de (mailing list archive)
State New
Headers show
Series debian: Integrate Debian/Ubuntu changes | expand

Commit Message

Bastian Germann Feb. 20, 2021, 12:16 p.m. UTC
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(-)

Comments

Dave Chinner Feb. 21, 2021, 3:59 a.m. UTC | #1
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.
Dimitri John Ledkov Feb. 21, 2021, 4:02 a.m. UTC | #2
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
Dave Chinner Feb. 21, 2021, 4:28 a.m. UTC | #3
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.
Dimitri John Ledkov Feb. 21, 2021, 4:32 a.m. UTC | #4
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.
Dave Chinner Feb. 21, 2021, 9:37 p.m. UTC | #5
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 mbox series

Patch

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" ;