Add compiler defenses flags

[pawpiatko]

It is essential to avoid buffer overflows and similar bugs as much as possible.

Add AX_CHECK_LINK_FLAG macro usage witch requires autoconf-archive package to be installed.

Add compiler flags: -D_FORTIFY_SOURCE - Compile-time protection against static sized buffer overflows, -fstack-protector-strong - Adds stack canaries to functions as safety checks against stack overwrites, -fPIE - Enables an ELF binary executable to be position independent, -fPIC - Ensures that shared object code that is built into shared libraries should be position independent code,.

Add linker flags: -pie - works together with gcc flag fPIE- please see its description, -z,relro - A security measure which makes some binary sections read-only, -z,now - Immediate Binding (Bindnow), -z,noexecstack - Prevents stack from being executable.

Add 'autoconf-archive' package dependency to github workflows. This package is requied by githab actions.

Fix compilation warnings.

[ktanska]

Verified

[mtkaczyk]

@tasleson It seems that we introduced an issue in Makefiles by overwriting AM_CFLAGS. We determined that the flags are lost. Here original code:

AM_CFLAGS = -I$(top_srcdir)/src/lib/include -I$(top_srcdir)/src -I$(top_srcdir)/config -I$(top_srcdir)/src/lib
{...}
ledmon_CFLAGS = $(AM_CFLAGS) $(LIBUDEV_CFLAGS)

Could you please take a look and see if the changes have sense?

[tasleson]

@tasleson It seems that we introduced an issue in Makefiles by overwriting AM_CFLAGS. We determined that the flags are lost. Here original code:

AM_CFLAGS = -I$(top_srcdir)/src/lib/include -I$(top_srcdir)/src -I$(top_srcdir)/config -I$(top_srcdir)/src/lib
{...}
ledmon_CFLAGS = $(AM_CFLAGS) $(LIBUDEV_CFLAGS)

Some observations on this

• Distributions will have their own preferred set of compiler/linker flags that are used during packaging which will override what the project is specifying
• We have some additional compiler warnings

  libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I../.. -D_DEBUG -D_GNU_SOURCE -D_DEFAULT_SOURCE -DDMALLOC_DISABLE -DBUILD_LABEL=\"\" -I../../src/lib/include -I../../src -I../../config -Wall -I../config -Wformat -Wformat-security -Werror=format-security -Werror=format-overflow=2 -Wno-strict-overflow -Wno-delete-null-pointer-checks -Werror=format-truncation=1 -Werror=shift-negative-value -Werror=alloca -Werror=missing-field-initializers -Werror=format-signedness -fvisibility=hidden -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fPIC -g -O2 -MT libledinternal_la-utils.lo -MD -MP -MF .deps/libledinternal_la-utils.Tpo -c utils.c  -fPIC -DPIC -o .libs/libledinternal_la-utils.o
  utils.c:775:26: warning: conflicting types for 'get_by_bits' due to enum/integer mismatch; have 'const struct ibpi2value *(const enum led_ibpi_pattern,  const struct ibpi2value *, int)' [-Wenum-int-mismatch]
  775 | const struct ibpi2value *get_by_##_name(const enum led_ibpi_pattern ibpi,                       \
    |                          ^~~~~~~
  utils.c:791:1: note: in expansion of macro 'IBPI2VALUE_GET_FN'
  791 | IBPI2VALUE_GET_FN(bits);
    | ^~~~~~~~~~~~~~~~~
  In file included from utils.c:49:
  utils.h:528:26: note: previous declaration of 'get_by_bits' with type 'const struct ibpi2value *(const unsigned int,  const struct ibpi2value *, int)'
  528 | const struct ibpi2value *get_by_bits(const unsigned int value,
    |                          ^~~~~~~~~~~
  utils.c:775:26: warning: conflicting types for 'get_by_value' due to enum/integer mismatch; have 'const struct ibpi2value *(const enum led_ibpi_pattern,  const struct ibpi2value *, int)' [-Wenum-int-mismatch]
  775 | const struct ibpi2value *get_by_##_name(const enum led_ibpi_pattern ibpi,                       \
    |                          ^~~~~~~
  utils.c:789:1: note: in expansion of macro 'IBPI2VALUE_GET_FN'
  789 | IBPI2VALUE_GET_FN(_name)                                                                \
    | ^~~~~~~~~~~~~~~~~
  utils.c:793:1: note: in expansion of macro 'COMPARE'
  793 | COMPARE(value);
    | ^~~~~~~
  utils.h:515:26: note: previous declaration of 'get_by_value' with type 'const struct ibpi2value *(const unsigned int,  const struct ibpi2value *, int)'
  515 | const struct ibpi2value *get_by_value(const unsigned int value,
    |                          ^~~~~~~~~~~~
  cc1: note: unrecognized command-line option '-Wno-delete-null-pointer-checks' may have been intended to silence earlier diagnostics

• This PR doesn't compile for me. The CI checks are not running configure and enabling the library and the building of the unit tests.

  ...
  Making all in tests
  make[2]: Entering directory '/home/tasleson/projects/ledmon/tests'
  gcc -DHAVE_CONFIG_H -I. -I..  -I../src/lib/include   -g -O2 -MT lib_unit_test-lib_unit_test.o -MD -MP -MF .deps/lib_unit_test-lib_unit_test.Tpo -c -o lib_unit_test-lib_unit_test.o `test -f 'lib_unit_test.c' || echo './'`lib_unit_test.c
  mv -f .deps/lib_unit_test-lib_unit_test.Tpo .deps/lib_unit_test-lib_unit_test.Po
  /bin/sh ../libtool  --tag=CC   --mode=link gcc  -g -O2 -pie -Wl,-z,relro -Wl,-z,now -Wl,-z,noexecstack  -o lib_unit_test lib_unit_test-lib_unit_test.o ../src/lib/libled.la -lcheck  -lsgutils2 -lrt 
  libtool: link: gcc -g -O2 -pie -Wl,-z -Wl,relro -Wl,-z -Wl,now -Wl,-z -Wl,noexecstack -o .libs/lib_unit_test lib_unit_test-lib_unit_test.o  ../src/lib/.libs/libled.so -lpci -lcheck -lsgutils2 -lrt
  /usr/bin/ld: lib_unit_test-lib_unit_test.o: relocation R_X86_64_32 against symbol `ctx' can not be used when making a PIE object; recompile with -fPIE
  /usr/bin/ld: failed to set dynamic section sizes: bad value
  collect2: error: ld returned 1 exit status
  make[2]: *** [Makefile:366: lib_unit_test] Error 1
  make[2]: Leaving directory '/home/tasleson/projects/ledmon/tests'
  make[1]: *** [Makefile:698: all-recursive] Error 1
  make[1]: Leaving directory '/home/tasleson/projects/ledmon'
  make: *** [Makefile:585: all] Error 2

• I did the following with this branch to get the above warning and error

  $ ./autogen.sh && ./configure --enable-systemd --enable-test --enable-library && V=1 make

[mtkaczyk]

Some observations on this

• Distributions will have their own preferred set of compiler/linker flags that are used during packaging which will override what the project is specifying
• We have some additional compiler warnings

We are obligated to follow Intel rules and those flags are required . Adding them here is the simplest solution but I agree that it is aggressive. Another option is to make a test compilation with more "controversial" flags and "-Werror" to break on any warning. I think that fits better because we can just ignore failing test until we get the warning resolved.

This PR doesn't compile for me. The CI checks are not running configure and enabling the library and the building of the unit tests.

Thanks, I think it is it something we should improve. Feel free to open the issue.

@pawpiatko please take a look.

[tasleson]

Thanks, I think it is it something we should improve. Feel free to open the issue.

Opened PR: https://github.com/intel/ledmon/pull/157

[pawpiatko]

tasleson could you please verify this change on your side? How can I reproduce warnings from utils files? Our compilation tests on GH actions are not catching them.

[tasleson]

tasleson could you please verify this change on your side? How can I reproduce warnings from utils files? Our compilation tests on GH actions are not catching them.

I'm using gcc (GCC) 13.2.1 20230728 (Red Hat 13.2.1-1). I think the compilers used for CI are older. Please see: https://github.com/intel/ledmon/pull/162

[ktanska]

@pawpiatko you can add gcc 13 to file https://github.com/intel/ledmon/blob/master/.github/workflows/review.yml and check the results then.

[pawpiatko]

@tasleson I'm still unable to reproduce it after adding gcc 13 to our CI. Could you please fix them out your side after submitting this change?

[tasleson]

@tasleson I'm still unable to reproduce it after adding gcc 13 to our CI. Could you please fix them out your side after submitting this change?

That is interesting. As I've mentioned a couple of times, I have PR up that includes this PR before the gcc-13 addition which also includes a fix for the utils warnings ref. 43915fc2c16b90db8a8ad1e88b4b8de441b77432

When this PR gets merged, I can fix up my PR for the other things I found and resubmit it.