[PATCH 0/2] ebpf: improves bpftool security

[PATCH 0/2] ebpf: improves bpftool security

[Xin Liu]

This patchset is designed to enhance the security of libbpf and bpf
tool, adding fPIC and fPIE options.

  - patch #1 add the fPIC options for dynamic library and static
    library.

  - patch #2 add the fPIE options for bpftool.

Xin Liu (2):
  libbpf: add fPIC option for static library
  bpftool: add fPIE option for bpftool

 tools/bpf/bpftool/Makefile | 1 +
 tools/lib/bpf/Makefile     | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

-- 
2.33.0

[PATCH 1/2] libbpf: add fPIC option for static library

[Xin Liu]

Some programs depned on libbpf.a(eg:bpftool). If libbpf.a miss -fPIC,
this will cause a similar error at compile time:

/usr/bin/ld: .../libbpf.a(libbpf-in.o): relocation
R_AARCH64_ADR_PREL_PG_HI21 against symbol `stderr@@GLIBC_2.17' which
may bind externally can not be used when making a sharedobject;
recompile with -fPIC

Use -fPIC for static library compilation to solve this problem.

Signed-off-by: Xin Liu <liuxin350@huawei.com>
---
 tools/lib/bpf/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 4c904ef0b47e..427e971f4fcd 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -91,9 +91,10 @@ override CFLAGS += $(INCLUDES)
 override CFLAGS += -fvisibility=hidden
 override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
 override CFLAGS += $(CLANG_CROSS_FLAGS)
+override CFLAGS += -fPIC
 
 # flags specific for shared library
-SHLIB_FLAGS := -DSHARED -fPIC
+SHLIB_FLAGS := -DSHARED
 
 ifeq ($(VERBOSE),1)
   Q =
-- 
2.33.0

[PATCH 2/2] bpftool: add fPIE option for bpftool

[Xin Liu]

Use the fPIE option to compile bpftool to improve bpftool security.

Signed-off-by: Xin Liu <liuxin350@huawei.com>
---
 tools/bpf/bpftool/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/bpf/bpftool/Makefile b/tools/bpf/bpftool/Makefile
index 4a95c017ad4c..d1cd21214388 100644
--- a/tools/bpf/bpftool/Makefile
+++ b/tools/bpf/bpftool/Makefile
@@ -72,6 +72,7 @@ bash_compdir ?= /usr/share/bash-completion/completions
 
 CFLAGS += -O2
 CFLAGS += -W -Wall -Wextra -Wno-unused-parameter -Wno-missing-field-initializers
+CFLAGS += -pie -fPIE
 CFLAGS += $(filter-out -Wswitch-enum -Wnested-externs,$(EXTRA_WARNINGS))
 CFLAGS += -DPACKAGE='"bpftool"' -D__EXPORTED_HEADERS__ \
 	-I$(or $(OUTPUT),.) \
-- 
2.33.0

Re: [PATCH 1/2] libbpf: add fPIC option for static library

[Quentin Monnet]

Sat Sep 24 2022 11:12:08 GMT+0100 ~ Xin Liu <liuxin350@huawei.com>
> Some programs depned on libbpf.a(eg:bpftool). If libbpf.a miss -fPIC,

Typo "depned"

> this will cause a similar error at compile time:
> 
> /usr/bin/ld: .../libbpf.a(libbpf-in.o): relocation
> R_AARCH64_ADR_PREL_PG_HI21 against symbol `stderr@@GLIBC_2.17' which
> may bind externally can not be used when making a sharedobject;
> recompile with -fPIC
> 
> Use -fPIC for static library compilation to solve this problem.
> 
> Signed-off-by: Xin Liu <liuxin350@huawei.com>
> ---
>  tools/lib/bpf/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index 4c904ef0b47e..427e971f4fcd 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -91,9 +91,10 @@ override CFLAGS += $(INCLUDES)
>  override CFLAGS += -fvisibility=hidden
>  override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
>  override CFLAGS += $(CLANG_CROSS_FLAGS)
> +override CFLAGS += -fPIC
>  
>  # flags specific for shared library
> -SHLIB_FLAGS := -DSHARED -fPIC
> +SHLIB_FLAGS := -DSHARED
>  
>  ifeq ($(VERBOSE),1)
>    Q =

Hi, the two patches look OK to me, but it would be nice to have a bit
more context on what the flags do other than “fixing this particular
issue” and how they improve bpftool security. It would also be
interesting to have a note on what it does on various architectures, my
understanding is that only some archs are supported (I read AArch64,
m68k, PowerPC and SPARC), I guess the flags are silently ignored on x86
for example?

Thanks,
Quentin

Re: [PATCH 1/2] libbpf: add fPIC option for static library

[Xin Liu]

On Tue, 27 Sep 2022 at 6:21:20 PM Quentin <quentin@isovalent.com> wrote:
> Sat Sep 24 2022 11:12:08 GMT+0100 ~ Xin Liu <liuxin350@huawei.com>
> > Some programs depned on libbpf.a(eg:bpftool). If libbpf.a miss -fPIC,
> 
> Typo "depned"
> 
> > this will cause a similar error at compile time:
> > 
> > /usr/bin/ld: .../libbpf.a(libbpf-in.o): relocation
> > R_AARCH64_ADR_PREL_PG_HI21 against symbol `stderr@@GLIBC_2.17' which
> > may bind externally can not be used when making a sharedobject;
> > recompile with -fPIC
> > 
> > Use -fPIC for static library compilation to solve this problem.
> > 
> > Signed-off-by: Xin Liu <liuxin350@huawei.com>
> > ---
> >  tools/lib/bpf/Makefile | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> > index 4c904ef0b47e..427e971f4fcd 100644
> > --- a/tools/lib/bpf/Makefile
> > +++ b/tools/lib/bpf/Makefile
> > @@ -91,9 +91,10 @@ override CFLAGS += $(INCLUDES)
> >  override CFLAGS += -fvisibility=hidden
> >  override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
> >  override CFLAGS += $(CLANG_CROSS_FLAGS)
> > +override CFLAGS += -fPIC
> >  
> >  # flags specific for shared library
> > -SHLIB_FLAGS := -DSHARED -fPIC
> > +SHLIB_FLAGS := -DSHARED
> >  
> >  ifeq ($(VERBOSE),1)
> >    Q =
> 
> Hi, the two patches look OK to me, but it would be nice to have a bit
> more context on what the flags do other than “fixing this particular
> issue” and how they improve bpftool security. It would also be
> interesting to have a note on what it does on various architectures, my
> understanding is that only some archs are supported (I read AArch64,
> m68k, PowerPC and SPARC), I guess the flags are silently ignored on x86
> for example?
> 
> Thanks,
> Quentin
> 
This advice is very useful to me. Thank you very much for your reply.
I'll fix it in the V2 version.

Thansk,
Xin Liu

Re: [PATCH 1/2] libbpf: add fPIC option for static library

[Andrii Nakryiko]

On Sat, Sep 24, 2022 at 3:13 AM Xin Liu <liuxin350@huawei.com> wrote:
>
> Some programs depned on libbpf.a(eg:bpftool). If libbpf.a miss -fPIC,
> this will cause a similar error at compile time:
>
> /usr/bin/ld: .../libbpf.a(libbpf-in.o): relocation
> R_AARCH64_ADR_PREL_PG_HI21 against symbol `stderr@@GLIBC_2.17' which
> may bind externally can not be used when making a sharedobject;
> recompile with -fPIC
>
> Use -fPIC for static library compilation to solve this problem.
>
> Signed-off-by: Xin Liu <liuxin350@huawei.com>
> ---
>  tools/lib/bpf/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index 4c904ef0b47e..427e971f4fcd 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -91,9 +91,10 @@ override CFLAGS += $(INCLUDES)
>  override CFLAGS += -fvisibility=hidden
>  override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
>  override CFLAGS += $(CLANG_CROSS_FLAGS)
> +override CFLAGS += -fPIC
>

It seems wrong to force -fPIC for static library just because in some
situations users might want to statically link their *shared* library
with *static* libbpf. It's a bit unconventional, even though I see
situations in which this might be useful.

But I don't think this can be a default. I see three possible solutions:

1. Do nothing. Let users specify EXTRA_CFLAGS=-fPIC if they need
position-independent static lib
2. Let packagers decide this (again, through EXTRA_CFLAGS or by
patching Makefile, whichever is best). Or maybe build both PIC and
non-PIC static libraries and package both?
3. Produce PIC and non-PIC libbpf.a libraries from libbpf's Makefile.

I'm not sure which one is the best answer, would be nice to hear
opinions of people who do the packaging and distribution of libbpf in
distros.

>  # flags specific for shared library
> -SHLIB_FLAGS := -DSHARED -fPIC
> +SHLIB_FLAGS := -DSHARED
>
>  ifeq ($(VERBOSE),1)
>    Q =
> --
> 2.33.0
>

Re: [PATCH 1/2] libbpf: add fPIC option for static library

[John Fastabend]

Andrii Nakryiko wrote:
> On Sat, Sep 24, 2022 at 3:13 AM Xin Liu <liuxin350@huawei.com> wrote:
> >
> > Some programs depned on libbpf.a(eg:bpftool). If libbpf.a miss -fPIC,
> > this will cause a similar error at compile time:
> >
> > /usr/bin/ld: .../libbpf.a(libbpf-in.o): relocation
> > R_AARCH64_ADR_PREL_PG_HI21 against symbol `stderr@@GLIBC_2.17' which
> > may bind externally can not be used when making a sharedobject;
> > recompile with -fPIC
> >
> > Use -fPIC for static library compilation to solve this problem.
> >
> > Signed-off-by: Xin Liu <liuxin350@huawei.com>
> > ---
> >  tools/lib/bpf/Makefile | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> > index 4c904ef0b47e..427e971f4fcd 100644
> > --- a/tools/lib/bpf/Makefile
> > +++ b/tools/lib/bpf/Makefile
> > @@ -91,9 +91,10 @@ override CFLAGS += $(INCLUDES)
> >  override CFLAGS += -fvisibility=hidden
> >  override CFLAGS += -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
> >  override CFLAGS += $(CLANG_CROSS_FLAGS)
> > +override CFLAGS += -fPIC
> >
> 
> It seems wrong to force -fPIC for static library just because in some
> situations users might want to statically link their *shared* library
> with *static* libbpf. It's a bit unconventional, even though I see
> situations in which this might be useful.
> 
> But I don't think this can be a default. I see three possible solutions:
> 
> 1. Do nothing. Let users specify EXTRA_CFLAGS=-fPIC if they need
> position-independent static lib
> 2. Let packagers decide this (again, through EXTRA_CFLAGS or by
> patching Makefile, whichever is best). Or maybe build both PIC and
> non-PIC static libraries and package both?
> 3. Produce PIC and non-PIC libbpf.a libraries from libbpf's Makefile.
> 
> I'm not sure which one is the best answer, would be nice to hear
> opinions of people who do the packaging and distribution of libbpf in
> distros.

Not a distro or pkg maintainer but my $.02 is I would just leave it
for 1 and 2.

> 
> >  # flags specific for shared library
> > -SHLIB_FLAGS := -DSHARED -fPIC
> > +SHLIB_FLAGS := -DSHARED
> >
> >  ifeq ($(VERBOSE),1)
> >    Q =
> > --
> > 2.33.0
> >