DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] mk: remove library search path from binary
@ 2019-11-12 13:15 Ferruh Yigit
  2019-11-18 15:14 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  2019-11-22 11:30 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  0 siblings, 2 replies; 8+ messages in thread
From: Ferruh Yigit @ 2019-11-12 13:15 UTC (permalink / raw)
  To: Thomas Monjalon, Andrew Rybchenko; +Cc: dev, stable, Bruce Richardson

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary
  2019-11-12 13:15 [dpdk-dev] [PATCH] mk: remove library search path from binary Ferruh Yigit
@ 2019-11-18 15:14 ` Thomas Monjalon
  2019-11-18 15:30   ` Bruce Richardson
  2019-11-21 17:12   ` Ferruh Yigit
  2019-11-22 11:30 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  1 sibling, 2 replies; 8+ messages in thread
From: Thomas Monjalon @ 2019-11-18 15:14 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Andrew Rybchenko, dev, Bruce Richardson, david.marchand

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary
  2019-11-18 15:14 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2019-11-18 15:30   ` Bruce Richardson
  2019-11-18 15:34     ` Bruce Richardson
  2019-11-21 17:12   ` Ferruh Yigit
  1 sibling, 1 reply; 8+ messages in thread
From: Bruce Richardson @ 2019-11-18 15:30 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Ferruh Yigit, Andrew Rybchenko, dev, david.marchand

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary
  2019-11-18 15:30   ` Bruce Richardson
@ 2019-11-18 15:34     ` Bruce Richardson
  0 siblings, 0 replies; 8+ messages in thread
From: Bruce Richardson @ 2019-11-18 15:34 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Ferruh Yigit, Andrew Rybchenko, dev, david.marchand

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary
  2019-11-18 15:14 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  2019-11-18 15:30   ` Bruce Richardson
@ 2019-11-21 17:12   ` Ferruh Yigit
  2019-11-21 21:17     ` Thomas Monjalon
  1 sibling, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2019-11-21 17:12 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: Andrew Rybchenko, dev, Bruce Richardson, david.marchand

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH] mk: remove library search path from binary
  2019-11-21 17:12   ` Ferruh Yigit
@ 2019-11-21 21:17     ` Thomas Monjalon
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2019-11-21 21:17 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: Andrew Rybchenko, dev, Bruce Richardson, david.marchand

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] mk: remove library search path from binary
  2019-11-12 13:15 [dpdk-dev] [PATCH] mk: remove library search path from binary Ferruh Yigit
  2019-11-18 15:14 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2019-11-22 11:30 ` Ferruh Yigit
  2019-11-25 22:11   ` Thomas Monjalon
  1 sibling, 1 reply; 8+ messages in thread
From: Ferruh Yigit @ 2019-11-22 11:30 UTC (permalink / raw)
  To: Thomas Monjalon, Andrew Rybchenko; +Cc: dev, stable, Bruce Richardson

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2] mk: remove library search path from binary
  2019-11-22 11:30 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
@ 2019-11-25 22:11   ` Thomas Monjalon
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2019-11-25 22:11 UTC (permalink / raw)
  To: Ferruh Yigit; +Cc: dev, Andrew Rybchenko, stable, Bruce Richardson

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




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-11-25 22:11 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12 13:15 [dpdk-dev] [PATCH] mk: remove library search path from binary Ferruh Yigit
2019-11-18 15:14 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2019-11-18 15:30   ` Bruce Richardson
2019-11-18 15:34     ` Bruce Richardson
2019-11-21 17:12   ` Ferruh Yigit
2019-11-21 21:17     ` Thomas Monjalon
2019-11-22 11:30 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2019-11-25 22:11   ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).