Re: [PATCH] configure.ac: warn if stack-protector not allowed

[Nicholas Vinson]

On 6/24/22 21:11, Glenn Washburn wrote:
> On Fri, 24 Jun 2022 14:12:44 -0400
> Nicholas Vinson <nvinson234@gmail.com> wrote:
>
> > On 6/24/22 12:28, Daniel Kiper wrote:
> > > Adding Chris...
> > >
> > > On Tue, Jun 14, 2022 at 06:19:00PM -0400, Nicholas Vinson wrote:
> > > > Previous version of configure.ac would error out when
> > > > --enable-stack-protector was given and a selected GRUB platform did not
> > > > support the flag.  The new behavior is to warn that the flag is not
> > > > supported and force disable it for that platform.
> > >
> > > Could you explain why do we need this change? I think it is safer to
> > > fail in this case than warn.
> >
> > What happened is I tried to modify Gentoo's GRUB ebuild so it would
> > unconditionally pass --enable-stack-protector to configure. However, the
> > ebuild also has a GRUB_PLATFORMS variable which allows it to build for
> > multiple GRUB platforms simultaneously for. For me the value of that
> > variable is set to "pc efi-64". Therefore, when I tried to install GRUB
> > the build would fail because the GRUB "pc" platform does not support
> > --enable-stack-protector.
> >
> > This essentially means that for any wrapper script that has some
> > variation of:
> >
> >       for p in SELECTED_GRUB_PLATFORMS; \
> >         do configure --enable-stack-protector \
> >         --with-platform${P} ... || die; \
> >       done
> >       make
> >
> > will fail to build if SELECTED_GRUB_PLATFORMS contains a platform that
> > does not support SSP.
> >
> > Therefore, the only way to work-around this issue is to modify the above
> > for-loop, so it conditionally passes '--enable-stack-protector' to
> > configure.
> >
> > If I modify the above example to have the conditional list, its behavior
> > is effectively the same as the change I'm proposing (sans warning
> > message). However, it does mean that the list of SSP supported platforms
> > is now in 2 places. One in the configure script and one in my
> > hypothetical for-loop. Furthermore, if the second list is not properly
> > maintained it could mistakenly disable SSP for a platform that later
> > gained support for it.
> >
> > In this particular case, the safety of warn vs error is about the same
> > and switching the statement to warn makes it easier to automate building
> > multiple GRUB platforms while passing '--enable-stack-protector' to
> > configure.
>
> I've run across this issue too and support doing something about it for
> the reasons listed here. I'm fine with the current implementation.
> However, I think a better solution would be to have
> '--enable-stack-protector=warn' issue a warning when it can not be
> enabled and '--enable-stack-protector' or
> '--enable-stack-protector=yes' be the current behavior.
>
> Glenn

Currently --enable-stack-protector accepts 'yes' and 'strong' as
arguments, and is used to select between '-fstack-protector' and
'-fstack-protector-strong'.

To keep that behavior and allow for the builder to choose between a
warning and an error might require a new switch.

Otherwise, I propose dropping -fstack-protector support and let
--enable-stack-protector=[yes,warn] always enable
-fstack-protector-strong.

Thanks,
Nicholas Vinson

> > Thanks,
> > Nicholas Vinson
> >
> > > > Signed-off-by: Nicholas Vinson <nvinson234@gmail.com>
> > > > ---
> > > >    configure.ac | 3 ++-
> > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/configure.ac b/configure.ac
> > > > index 57fb70945..9bdc102b2 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -1349,7 +1349,8 @@ if test "x$enable_stack_protector" = xno; then
> > > >        TARGET_CFLAGS="$TARGET_CFLAGS -fno-stack-protector"
> > > >      fi
> > > >    elif test "x$platform" != xefi; then
> > > > -  AC_MSG_ERROR([--enable-stack-protector is only supported on EFI platforms])
> > > > +  AC_MSG_WARN([--enable-stack-protector is only supported on EFI platforms])
> > > > +  enable_stack_protector=no
> > > >    elif test "x$ssp_global_possible" != xyes; then
> > > >      AC_MSG_ERROR([--enable-stack-protector is not supported (compiler doesn't 
> > > > support -mstack-protector-guard=global)])
> > > >    else
> > > > --
> > > > 2.35.1
> > >
> > > Daniel
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel