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