mk: remove library search path from binary
diff mbox series

Message ID 20191112131556.16668-1-ferruh.yigit@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • mk: remove library search path from binary
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-compilation success Compile Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch success coding style OK

Commit Message

Ferruh Yigit Nov. 12, 2019, 1:15 p.m. UTC
This patch functionally reverts the patch in fixes line to not have any
hardcoded library path in the final binary for the security reasons, in
case this binary distributed to production environment.

RPATH only added in RTE_DEVEL_BUILD case and this binary shouldn't
distributed, but still removing it to be cautious.

Fixes: 8919f73bcbaa ("mk: add build directory to library search path")
Cc: stable@dpdk.org

Suggested-by: Bruce Richardson <bruce.richardson@intel.com>
Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 devtools/test-null.sh | 1 +
 mk/rte.app.mk         | 4 ----
 2 files changed, 1 insertion(+), 4 deletions(-)

Comments

Thomas Monjalon Nov. 18, 2019, 3:14 p.m. UTC | #1
12/11/2019 14:15, Ferruh Yigit:
> This patch functionally reverts the patch in fixes line to not have any
> hardcoded library path in the final binary for the security reasons, in
> case this binary distributed to production environment.

What about meson?
There are these rpaths:
	$ORIGIN/../lib
	$ORIGIN/../drivers


> RPATH only added in RTE_DEVEL_BUILD case and this binary shouldn't
> distributed, but still removing it to be cautious.

For convenience, we could keep adding rpath for internal apps.


> --- a/devtools/test-null.sh
> +++ b/devtools/test-null.sh

>  if ldd $testpmd | grep -q librte_ ; then
> +	export LD_LIBRARY_PATH=$build/lib:$LD_LIBRARY_PATH
>  	libs='-d librte_mempool_ring.so -d librte_pmd_null.so'


There is an issue in this change, because $build may be undefined.
It can be fixed with adding this line:

+[ -f "$testpmd" ] && build=$(dirname $(dirname $testpmd))
 [ -f "$testpmd" ] || testpmd=$build/app/dpdk-testpmd
 [ -f "$testpmd" ] || testpmd=$build/app/testpmd
Bruce Richardson Nov. 18, 2019, 3:30 p.m. UTC | #2
On Mon, Nov 18, 2019 at 04:14:54PM +0100, Thomas Monjalon wrote:
> 12/11/2019 14:15, Ferruh Yigit:
> > This patch functionally reverts the patch in fixes line to not have any
> > hardcoded library path in the final binary for the security reasons, in
> > case this binary distributed to production environment.
> 
> What about meson?
> There are these rpaths:
> 	$ORIGIN/../lib
> 	$ORIGIN/../drivers
> 

Meson uses relative paths based off the file location "$ORIGIN" as you see
above. This avoids having a user's home path in the search directories.

However, meson also adjusts the rpath on install, so if you run
test-meson-builds.sh and check the rpath on
build-x64-default/app/dpdk-testpmd and compare against
build-x86-default/install-root/usr/local/bin/dpdk-testpmd you'll see they
are different, with the latter having the final install path encoded in it.
If we do want to control these, they can be set for binaries using the
"build_rpath" and "install_rpath" parameters, though I think the current
values are ok.

/Bruce
Bruce Richardson Nov. 18, 2019, 3:34 p.m. UTC | #3
On Mon, Nov 18, 2019 at 03:30:32PM +0000, Bruce Richardson wrote:
> On Mon, Nov 18, 2019 at 04:14:54PM +0100, Thomas Monjalon wrote:
> > 12/11/2019 14:15, Ferruh Yigit:
> > > This patch functionally reverts the patch in fixes line to not have any
> > > hardcoded library path in the final binary for the security reasons, in
> > > case this binary distributed to production environment.
> > 
> > What about meson?
> > There are these rpaths:
> > 	$ORIGIN/../lib
> > 	$ORIGIN/../drivers
> > 
> 
> Meson uses relative paths based off the file location "$ORIGIN" as you see
> above. This avoids having a user's home path in the search directories.
> 
> However, meson also adjusts the rpath on install, so if you run
> test-meson-builds.sh and check the rpath on
> build-x64-default/app/dpdk-testpmd and compare against
> build-x86-default/install-root/usr/local/bin/dpdk-testpmd you'll see they
> are different, with the latter having the final install path encoded in it.
> If we do want to control these, they can be set for binaries using the
> "build_rpath" and "install_rpath" parameters, though I think the current
> values are ok.
> 
> /Bruce

Apologies for the self-reply, but forgot to include the reference to the
relevant parameters [1], and they should apply for both executables and
libraries [2].

[1] https://mesonbuild.com/Reference-manual.html#executable
[2] https://mesonbuild.com/Reference-manual.html#library
Ferruh Yigit Nov. 21, 2019, 5:12 p.m. UTC | #4
On 11/18/2019 3:14 PM, Thomas Monjalon wrote:
> 12/11/2019 14:15, Ferruh Yigit:
>> This patch functionally reverts the patch in fixes line to not have any
>> hardcoded library path in the final binary for the security reasons, in
>> case this binary distributed to production environment.
> 
> What about meson?
> There are these rpaths:
> 	$ORIGIN/../lib
> 	$ORIGIN/../drivers
> 
> 
>> RPATH only added in RTE_DEVEL_BUILD case and this binary shouldn't
>> distributed, but still removing it to be cautious.
> 
> For convenience, we could keep adding rpath for internal apps.

This was the main intention, but the concern is someone unaware of this
capability and distributes a binary that we think it will be internal.

> 
> 
>> --- a/devtools/test-null.sh
>> +++ b/devtools/test-null.sh
> 
>>  if ldd $testpmd | grep -q librte_ ; then
>> +	export LD_LIBRARY_PATH=$build/lib:$LD_LIBRARY_PATH
>>  	libs='-d librte_mempool_ring.so -d librte_pmd_null.so'
> 
> 
> There is an issue in this change, because $build may be undefined.
> It can be fixed with adding this line:
> 
> +[ -f "$testpmd" ] && build=$(dirname $(dirname $testpmd))
>  [ -f "$testpmd" ] || testpmd=$build/app/dpdk-testpmd
>  [ -f "$testpmd" ] || testpmd=$build/app/testpmd

'build' is already defined as following at the beginning of the script
build=${1:-build}

And if 'build' is wrong/missing, script can't reach to this line at all, because
'testpmd' path found based on 'build' and if 'testpmd' not found, script will exit.

Can you please give more detail what is problem with 'build'?
Thomas Monjalon Nov. 21, 2019, 9:17 p.m. UTC | #5
21/11/2019 18:12, Ferruh Yigit:
> On 11/18/2019 3:14 PM, Thomas Monjalon wrote:
> > 12/11/2019 14:15, Ferruh Yigit:
> >> This patch functionally reverts the patch in fixes line to not have any
> >> hardcoded library path in the final binary for the security reasons, in
> >> case this binary distributed to production environment.
> > 
> > What about meson?
> > There are these rpaths:
> > 	$ORIGIN/../lib
> > 	$ORIGIN/../drivers
> > 
> > 
> >> RPATH only added in RTE_DEVEL_BUILD case and this binary shouldn't
> >> distributed, but still removing it to be cautious.
> > 
> > For convenience, we could keep adding rpath for internal apps.
> 
> This was the main intention, but the concern is someone unaware of this
> capability and distributes a binary that we think it will be internal.

Internal apps are only for developers.
I don't see how there could be a security issue.

> >> --- a/devtools/test-null.sh
> >> +++ b/devtools/test-null.sh
> > 
> >>  if ldd $testpmd | grep -q librte_ ; then
> >> +	export LD_LIBRARY_PATH=$build/lib:$LD_LIBRARY_PATH
> >>  	libs='-d librte_mempool_ring.so -d librte_pmd_null.so'
> > 
> > 
> > There is an issue in this change, because $build may be undefined.
> > It can be fixed with adding this line:
> > 
> > +[ -f "$testpmd" ] && build=$(dirname $(dirname $testpmd))
> >  [ -f "$testpmd" ] || testpmd=$build/app/dpdk-testpmd
> >  [ -f "$testpmd" ] || testpmd=$build/app/testpmd
> 
> 'build' is already defined as following at the beginning of the script
> build=${1:-build}

Yes, but $1 can be the testpmd path as well, so $build is meaningless.

> And if 'build' is wrong/missing, script can't reach to this line at all, because
> 'testpmd' path found based on 'build' and if 'testpmd' not found, script will exit.

No, $testpmd can be defined from $1, not based on $build.
You missed this comment:

build=${1:-build} # first argument can be the build directory
testpmd=$1 # or first argument can be the testpmd path

> Can you please give more detail what is problem with 'build'?

If the testpmd path is directly passed as first parameter,
build directory is not known.
That's why I suggest getting it with $(dirname $(dirname $testpmd)).

Patch
diff mbox series

diff --git a/devtools/test-null.sh b/devtools/test-null.sh
index 9f9a459f7..0fbb403e2 100755
--- a/devtools/test-null.sh
+++ b/devtools/test-null.sh
@@ -19,6 +19,7 @@  if [ ! -f "$testpmd" ] ; then
 fi
 
 if ldd $testpmd | grep -q librte_ ; then
+	export LD_LIBRARY_PATH=$build/lib:$LD_LIBRARY_PATH
 	libs='-d librte_mempool_ring.so -d librte_pmd_null.so'
 else
 	libs=
diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index 683e3a4e3..077eaee06 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -379,10 +379,6 @@  filter-libs = \
 
 LDLIBS := $(call filter-libs,$(LDLIBS))
 
-ifeq ($(RTE_DEVEL_BUILD)$(CONFIG_RTE_BUILD_SHARED_LIB),yy)
-LDFLAGS += -rpath=$(RTE_SDK_BIN)/lib
-endif
-
 MAPFLAGS = -Map=$@.map --cref
 
 .PHONY: all