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

[Glenn Washburn]

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

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