[dpdk-dev] [PATCH] mk: remove library search path from binary

[dpdk-dev] [PATCH] mk: remove library search path from binary

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

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

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary

[Thomas Monjalon]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary

[Bruce Richardson]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary

[Bruce Richardson]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary

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

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

Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary

[Thomas Monjalon]

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

[dpdk-dev] [PATCH v2] mk: remove library search path from binary

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

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>
---
v2:
* set 'build' if provided argument is testpmd path
---
 devtools/test-null.sh | 2 ++
 mk/rte.app.mk         | 4 ----
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/devtools/test-null.sh b/devtools/test-null.sh
index 9f9a459f7..afcd8bb29 100755
--- a/devtools/test-null.sh
+++ b/devtools/test-null.sh
@@ -11,6 +11,7 @@ coremask=${2:-3} # default using cores 0 and 1
 eal_options=$3
 testpmd_options=$4
 
+[ -f "$testpmd" ] && build=$(dirname $(dirname $testpmd))
 [ -f "$testpmd" ] || testpmd=$build/app/dpdk-testpmd
 [ -f "$testpmd" ] || testpmd=$build/app/testpmd
 if [ ! -f "$testpmd" ] ; then
@@ -19,6 +20,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 a278552c6..05ea034b9 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
-- 
2.21.0

Re: [dpdk-dev] [PATCH v2] mk: remove library search path from binary

[Thomas Monjalon]

22/11/2019 12:30, 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.
> 
> 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>

Applied, thanks