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

[Daniel Kiper]

On Sat, Jun 25, 2022 at 03:07:09AM -0400, Nicholas Vinson wrote:
> 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.

If Chris do not object I am OK with this (please CC Chris when you sent v2).
However, please add an explanation to the commit message why this patch is
needed. I think you can copy more or less text from your first reply in
the thread.

Daniel