diff mbox

[v9,04/15] busybox: applets as individual binaries

Message ID 1436905227-26937-5-git-send-email-clayton.shotwell@rockwellcollins.com
State Changes Requested
Headers show

Commit Message

Clayton Shotwell July 14, 2015, 8:20 p.m. UTC 
The individual binaries option of busybox allows for the applets
that would usually be symlinks to be built as individual applications
that link against a shared library.

This feature is needed for SELinux to allow the applications to run
under the correct SELinux context.

The patch being added allows the individual applications to be
installed and will be upstreamed to the busybox developers.

The initial work for this change was done by Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>.

Signed-off-by: Clayton Shotwell <clayton.shotwell@rockwellcollins.com>
Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com>
Reviewed-by: Samuel Martin <s.martin49@gmail.com>

---
Changes v8 -> v9:
  - No changes

Changes v7 -> v8:
  - Changed individual binaries comment to be !BR2_bfin (Suggested by
    Samuel)

Changes v6 -> v7:
  - No changes

Changes v5 -> v6:
  - No changes

Changes v4 -> v5:
  - Renamed to follow latest patch naming convention (Matt W.)
  - Updated to use BR2_STATIC_LIBS instead of old PREFERRED (Matt W.)
  - Added depends to make sure bfin can't build shared lib
    busybox lib for individual binary use.  Looks like shared
    lib creation doesn't error out but the objects don't get
    placed into the elf.  Then the trylink fails on linking
    the first individual applet. (Matt W.)
  - Made suid permissions setting dynamic for applets actually being
    installed (Clayton S.)

Changes v1 -> v4:
  - Did not exist
---
 ...s-Add-installation-of-individual-binaries.patch | 103 +++++++++++++++++++++
 package/busybox/Config.in                          |   9 ++
 package/busybox/busybox.mk                         |  41 ++++++++
 3 files changed, 153 insertions(+)
 create mode 100644 package/busybox/0002-applets-Add-installation-of-individual-binaries.patch

Comments

Thomas Petazzoni July 18, 2015, 12:46 p.m. UTC | #1 
Dear Clayton Shotwell,

On Tue, 14 Jul 2015 15:20:16 -0500, Clayton Shotwell wrote:

> diff --git a/package/busybox/0002-applets-Add-installation-of-individual-binaries.patch b/package/busybox/0002-applets-Add-installation-of-individual-binaries.patch
> new file mode 100644
> index 0000000..ae0e654
> --- /dev/null
> +++ b/package/busybox/0002-applets-Add-installation-of-individual-binaries.patch
> @@ -0,0 +1,103 @@
> +From 3451b55054a6fe2073a21301938802a27dec835d Mon Sep 17 00:00:00 2001
> +From: Clayton Shotwell <clshotwe@rockwellcollins.com>
> +Date: Mon, 16 Dec 2013 14:45:33 -0600
> +Subject: [PATCH 5/5] applets: Add installation of individual binaries

This PATCH 5/5 is not desirable, use git format-patch -N when
generating patches. Also, now that the patch is upstream, maybe you
could backport the upstream patch, with has Denys Signed-off-by, and
add an indication that the patch is upstream.

(Note: I would have done all that myself, but I have some other
comments below)

> diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
> index 6b2abca..4942e75 100644
> --- a/package/busybox/busybox.mk
> +++ b/package/busybox/busybox.mk
> @@ -50,9 +50,37 @@ BUSYBOX_KCONFIG_FRAGMENT_FILES = $(call qstrip,$(BR2_PACKAGE_BUSYBOX_CONFIG_FRAG
>  BUSYBOX_KCONFIG_EDITORS = menuconfig xconfig gconfig
>  BUSYBOX_KCONFIG_OPTS = $(BUSYBOX_MAKE_OPTS)
>  
> +ifeq ($(BR2_PACKAGE_BUSYBOX_INDIVIDUAL_BINARIES),y)
> +define BUSYBOX_PERMISSIONS
> +	/usr/share/udhcpc/default.script f 755  0  0 - - - - -
> +endef
> +
> +# Set permissions on all applets with BB_SUID_REQUIRE and BB_SUID_MAYBE. The
> +# permissions are pulled from the applets.h file that is generated during
> +# the build and used to determine all of the possible applets. The permissions
> +# file is generated and added to the list of device tables used by makedevs to
> +# set file permissions.
> +define BUSYBOX_MAKEDEV_PERMISSIONS
> +	if [ -f $(@D)/.buildroot_permissions ]; then \
> +		rm $(@D)/.buildroot_permissions; \
> +	fi; \
> +	touch $(@D)/.buildroot_permissions; \
> +	for app in `grep -r -e "APPLET.*BB_SUID_REQUIRE\|APPLET.*BB_SUID_MAYBE" $(@D)/include/applets.h \
> +			| sed -e 's/,.*//' -e 's/.*(//'`; \
> +	do \
> +		temp=`grep -w $${app} $(@D)/busybox.links`; \
> +		if [ -n "$${temp}" ]; then \
> +			echo "$${temp} f 4755 0  0 - - - - -" >> $(@D)/.buildroot_permissions; \
> +		fi; \
> +	done
> +endef
> +BUSYBOX_POST_INSTALL_TARGET_HOOKS += BUSYBOX_MAKEDEV_PERMISSIONS
> +BR2_ROOTFS_DEVICE_TABLE += $(BUSYBOX_DIR)/.buildroot_permissions
> +else

I'm sorry but I don't like this. I don't think any Buildroot package
*modifies* a BR2_<something> option, that's really a hack. I think the
only reasonable solution is to have a real permission table, containing
the list of all applets that may need SUID root. However, I don't
remember if we error out when a file mentioned in a permission table
does not exist. I've added Yann in Cc to discuss that further. Maybe we
need a special syntax in the permission table to say "change the
permission of this file if it exists, otherwise ignore".

> +ifeq ($(BR2_PACKAGE_BUSYBOX_INDIVIDUAL_BINARIES),y)
> +define BUSYBOX_CONFIGURE_INDIVIDUAL_BINARIES
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_BUILD_LIBBUSYBOX,$(BUSYBOX_BUILD_CONFIG))
> +	$(call KCONFIG_ENABLE_OPT,CONFIG_FEATURE_INDIVIDUAL,$(BUSYBOX_BUILD_CONFIG))
> +endef
> +
> +define BUSYBOX_INSTALL_INDIVIDUAL_BINARIES
> +	rm -f $(TARGET_DIR)/bin/busybox
> +endef

This is not really installing individual binaries, but removing the
main Busybox binary. Could you rename the macro accordingly?

Long term, it'd be nicer if the Busybox build system itself would not
install bin/busybox when individual binaries are selected.

Could you work on the above issues? Really, only the permission table
one is annoying, the rest is trivial.

Thomas
Yann E. MORIN July 18, 2015, 2:26 p.m. UTC | #2 
Clayton, Thomas, All,

On 2015-07-18 14:46 +0200, Thomas Petazzoni spake thusly:
> On Tue, 14 Jul 2015 15:20:16 -0500, Clayton Shotwell wrote:
[--SNIP--]
> > diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
> > index 6b2abca..4942e75 100644
> > --- a/package/busybox/busybox.mk
> > +++ b/package/busybox/busybox.mk
> > @@ -50,9 +50,37 @@ BUSYBOX_KCONFIG_FRAGMENT_FILES = $(call qstrip,$(BR2_PACKAGE_BUSYBOX_CONFIG_FRAG
> >  BUSYBOX_KCONFIG_EDITORS = menuconfig xconfig gconfig
> >  BUSYBOX_KCONFIG_OPTS = $(BUSYBOX_MAKE_OPTS)
> >  
> > +ifeq ($(BR2_PACKAGE_BUSYBOX_INDIVIDUAL_BINARIES),y)
> > +define BUSYBOX_PERMISSIONS
> > +	/usr/share/udhcpc/default.script f 755  0  0 - - - - -
> > +endef
> > +
> > +# Set permissions on all applets with BB_SUID_REQUIRE and BB_SUID_MAYBE. The
> > +# permissions are pulled from the applets.h file that is generated during
> > +# the build and used to determine all of the possible applets. The permissions
> > +# file is generated and added to the list of device tables used by makedevs to
> > +# set file permissions.
> > +define BUSYBOX_MAKEDEV_PERMISSIONS
> > +	if [ -f $(@D)/.buildroot_permissions ]; then \
> > +		rm $(@D)/.buildroot_permissions; \
> > +	fi; \
> > +	touch $(@D)/.buildroot_permissions; \
> > +	for app in `grep -r -e "APPLET.*BB_SUID_REQUIRE\|APPLET.*BB_SUID_MAYBE" $(@D)/include/applets.h \
> > +			| sed -e 's/,.*//' -e 's/.*(//'`; \
> > +	do \
> > +		temp=`grep -w $${app} $(@D)/busybox.links`; \
> > +		if [ -n "$${temp}" ]; then \
> > +			echo "$${temp} f 4755 0  0 - - - - -" >> $(@D)/.buildroot_permissions; \
> > +		fi; \
> > +	done
> > +endef
> > +BUSYBOX_POST_INSTALL_TARGET_HOOKS += BUSYBOX_MAKEDEV_PERMISSIONS
> > +BR2_ROOTFS_DEVICE_TABLE += $(BUSYBOX_DIR)/.buildroot_permissions
> > +else
> 
> I'm sorry but I don't like this. I don't think any Buildroot package
> *modifies* a BR2_<something> option, that's really a hack. I think the
> only reasonable solution is to have a real permission table, containing
> the list of all applets that may need SUID root. However, I don't
> remember if we error out when a file mentioned in a permission table
> does not exist. I've added Yann in Cc to discuss that further. Maybe we
> need a special syntax in the permission table to say "change the
> permission of this file if it exists, otherwise ignore".

Well, I had a cursory look at makedev.c, and it seems a missing file is
treated as an error:

  482         } else if (type == 'f') {
  483             struct stat st;
  484             if ((stat(full_name, &st) < 0 || !S_ISREG(st.st_mode))) {
  485                 bb_perror_msg("line %d: regular file '%s' does not exist", linenum, full_name);
  486                 ret = EXIT_FAILURE;
  487                 goto loop;
  488             }

So, either we filter-out missing applets (Hurck!) or we add a mode to
makedev to ignore missing files, something along the lines of:

    diff --git a/package/makedevs/makedevs.c b/package/makedevs/makedevs.c
    index 53ff6fe..8a66fa3 100644
    --- a/package/makedevs/makedevs.c
    +++ b/package/makedevs/makedevs.c
    @@ -479,9 +479,11 @@ int main(int argc, char **argv)
     				ret = EXIT_FAILURE;
     				goto loop;
     			}
    -		} else if (type == 'f') {		
    +		} else if (type == 'f' || type == 'F') {
     			struct stat st;
     			if ((stat(full_name, &st) < 0 || !S_ISREG(st.st_mode))) {
    +				if (type == 'F')
    +					continue; /* Ignore optional files */
     				bb_perror_msg("line %d: regular file '%s' does not exist", linenum, full_name);
     				ret = EXIT_FAILURE;
     				goto loop;

(Of course 'F' is just a place-holder, we might need a better type...)

Totally untested; needs update in the documentation as well. Might be
worth sending to Busybox too (like for the recursive option) sicne
Busybox is our upstream for that makedev.c file (but was a long time
ago, might no longer apply cleanly...)

Regards,
Yann E. MORIN.
diff mbox

Patch

diff --git a/package/busybox/0002-applets-Add-installation-of-individual-binaries.patch b/package/busybox/0002-applets-Add-installation-of-individual-binaries.patch
new file mode 100644
index 0000000..ae0e654
--- /dev/null
+++ b/package/busybox/0002-applets-Add-installation-of-individual-binaries.patch
@@ -0,0 +1,103 @@ 
+From 3451b55054a6fe2073a21301938802a27dec835d Mon Sep 17 00:00:00 2001
+From: Clayton Shotwell <clshotwe@rockwellcollins.com>
+Date: Mon, 16 Dec 2013 14:45:33 -0600
+Subject: [PATCH 5/5] applets: Add installation of individual binaries
+
+Adding support to install individual binaries if the option is
+enabled. This also installs the shared libbusybox.so.* library.
+
+Signed-off-by: Clayton Shotwell <clshotwe@rockwellcollins.com>
+---
+ Makefile.custom    |    4 ++++
+ applets/install.sh |   26 ++++++++++++++++++++++++--
+ 2 files changed, 28 insertions(+), 2 deletions(-)
+
+diff --git a/Makefile.custom b/Makefile.custom
+index 6da79e6..e4dc4dc 100644
+--- a/Makefile.custom
++++ b/Makefile.custom
+@@ -23,6 +23,10 @@ ifeq ($(CONFIG_INSTALL_SH_APPLET_SCRIPT_WRAPPER),y)
+ INSTALL_OPTS:= --scriptwrapper
+ endif
+ endif
++ifeq ($(CONFIG_FEATURE_INDIVIDUAL),y)
++INSTALL_OPTS:= --binaries
++LIBBUSYBOX_SONAME:= 0_lib/libbusybox.so.$(BB_VER)
++endif
+ install: $(srctree)/applets/install.sh busybox busybox.links
+ 	$(Q)DO_INSTALL_LIBS="$(strip $(LIBBUSYBOX_SONAME) $(DO_INSTALL_LIBS))" \
+ 		$(SHELL) $< $(CONFIG_PREFIX) $(INSTALL_OPTS)
+diff --git a/applets/install.sh b/applets/install.sh
+index 95b4719..d01c98d 100755
+--- a/applets/install.sh
++++ b/applets/install.sh
+@@ -5,19 +5,26 @@ export LC_CTYPE=POSIX
+ 
+ prefix=$1
+ if [ -z "$prefix" ]; then
+-	echo "usage: applets/install.sh DESTINATION [--symlinks/--hardlinks/--scriptwrapper]"
++	echo "usage: applets/install.sh DESTINATION [--symlinks/--hardlinks/--binaries/--scriptwrapper]"
+ 	exit 1
+ fi
+ 
++# Source the configuration
++. ./.config
++
+ h=`sort busybox.links | uniq`
+ 
++sharedlib_dir="0_lib"
++
+ linkopts=""
+ scriptwrapper="n"
++binaries="n"
+ cleanup="0"
+ noclobber="0"
+ case "$2" in
+ 	--hardlinks)     linkopts="-f";;
+ 	--symlinks)      linkopts="-fs";;
++	--binaries)      binaries="y";;
+ 	--scriptwrapper) scriptwrapper="y";swrapall="y";;
+ 	--sw-sh-hard)    scriptwrapper="y";linkopts="-f";;
+ 	--sw-sh-sym)     scriptwrapper="y";linkopts="-fs";;
+@@ -40,8 +47,9 @@ if [ -n "$DO_INSTALL_LIBS" ] && [ "$DO_INSTALL_LIBS" != "n" ]; then
+ 	for i in $DO_INSTALL_LIBS; do
+ 		rm -f "$prefix/$libdir/$i" || exit 1
+ 		if [ -f "$i" ]; then
++			echo "   Installing $i to the target at $prefix/$libdir/"
+ 			cp -pPR "$i" "$prefix/$libdir/" || exit 1
+-			chmod 0644 "$prefix/$libdir/$i" || exit 1
++			chmod 0644 "$prefix/$libdir/`basename $i`" || exit 1
+ 		fi
+ 	done
+ fi
+@@ -68,6 +76,7 @@ install -m 755 busybox "$prefix/bin/busybox" || exit 1
+ 
+ for i in $h; do
+ 	appdir=`dirname "$i"`
++	app=`basename "$i"`
+ 	mkdir -p "$prefix/$appdir" || exit 1
+ 	if [ "$scriptwrapper" = "y" ]; then
+ 		if [ "$swrapall" != "y" ] && [ "$i" = "/bin/sh" ]; then
+@@ -78,6 +87,19 @@ for i in $h; do
+ 			chmod +x "$prefix/$i"
+ 		fi
+ 		echo "	$prefix/$i"
++	elif [ "$binaries" = "y" ]; then
++		# Copy the binary over rather
++		if [ -e $sharedlib_dir/$app ]; then
++			if [ "$noclobber" = "0" ] || [ ! -e "$prefix/$i" ]; then
++				echo "   Copying $sharedlib_dir/$app to $prefix/$i"
++				cp -pPR $sharedlib_dir/$app $prefix/$i || exit 1
++			else
++				echo "  $prefix/$i already exists"
++			fi
++		else
++			echo "Error: Could not find $sharedlib_dir/$app"
++			exit 1
++		fi
+ 	else
+ 		if [ "$2" = "--hardlinks" ]; then
+ 			bb_path="$prefix/bin/busybox"
+-- 
+1.7.1
+
diff --git a/package/busybox/Config.in b/package/busybox/Config.in
index 6847a60..b3303c0 100644
--- a/package/busybox/Config.in
+++ b/package/busybox/Config.in
@@ -32,6 +32,15 @@  config BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
 	  Show packages in menuconfig that are potentially also provided
 	  by busybox.
 
+config BR2_PACKAGE_BUSYBOX_INDIVIDUAL_BINARIES
+	bool "Individual binaries"
+	depends on !BR2_STATIC_LIBS
+	depends on !BR2_bfin # libbusybox.so link issue
+
+comment "Busybox individual binaries depends on dynamic libraries"
+	depends on BR2_STATIC_LIBS
+	depends on !BR2_bfin
+
 config BR2_PACKAGE_BUSYBOX_WATCHDOG
 	bool "Install the watchdog daemon startup script"
 	help
diff --git a/package/busybox/busybox.mk b/package/busybox/busybox.mk
index 6b2abca..4942e75 100644
--- a/package/busybox/busybox.mk
+++ b/package/busybox/busybox.mk
@@ -50,9 +50,37 @@  BUSYBOX_KCONFIG_FRAGMENT_FILES = $(call qstrip,$(BR2_PACKAGE_BUSYBOX_CONFIG_FRAG
 BUSYBOX_KCONFIG_EDITORS = menuconfig xconfig gconfig
 BUSYBOX_KCONFIG_OPTS = $(BUSYBOX_MAKE_OPTS)
 
+ifeq ($(BR2_PACKAGE_BUSYBOX_INDIVIDUAL_BINARIES),y)
+define BUSYBOX_PERMISSIONS
+	/usr/share/udhcpc/default.script f 755  0  0 - - - - -
+endef
+
+# Set permissions on all applets with BB_SUID_REQUIRE and BB_SUID_MAYBE. The
+# permissions are pulled from the applets.h file that is generated during
+# the build and used to determine all of the possible applets. The permissions
+# file is generated and added to the list of device tables used by makedevs to
+# set file permissions.
+define BUSYBOX_MAKEDEV_PERMISSIONS
+	if [ -f $(@D)/.buildroot_permissions ]; then \
+		rm $(@D)/.buildroot_permissions; \
+	fi; \
+	touch $(@D)/.buildroot_permissions; \
+	for app in `grep -r -e "APPLET.*BB_SUID_REQUIRE\|APPLET.*BB_SUID_MAYBE" $(@D)/include/applets.h \
+			| sed -e 's/,.*//' -e 's/.*(//'`; \
+	do \
+		temp=`grep -w $${app} $(@D)/busybox.links`; \
+		if [ -n "$${temp}" ]; then \
+			echo "$${temp} f 4755 0  0 - - - - -" >> $(@D)/.buildroot_permissions; \
+		fi; \
+	done
+endef
+BUSYBOX_POST_INSTALL_TARGET_HOOKS += BUSYBOX_MAKEDEV_PERMISSIONS
+BR2_ROOTFS_DEVICE_TABLE += $(BUSYBOX_DIR)/.buildroot_permissions
+else
 define BUSYBOX_PERMISSIONS
 	/bin/busybox                     f 4755 0  0 - - - - -
 endef
+endif
 
 # If mdev will be used for device creation enable it and copy S10mdev to /etc/init.d
 ifeq ($(BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV),y)
@@ -141,6 +169,17 @@  define BUSYBOX_SET_INIT
 endef
 endif
 
+ifeq ($(BR2_PACKAGE_BUSYBOX_INDIVIDUAL_BINARIES),y)
+define BUSYBOX_CONFIGURE_INDIVIDUAL_BINARIES
+	$(call KCONFIG_ENABLE_OPT,CONFIG_BUILD_LIBBUSYBOX,$(BUSYBOX_BUILD_CONFIG))
+	$(call KCONFIG_ENABLE_OPT,CONFIG_FEATURE_INDIVIDUAL,$(BUSYBOX_BUILD_CONFIG))
+endef
+
+define BUSYBOX_INSTALL_INDIVIDUAL_BINARIES
+	rm -f $(TARGET_DIR)/bin/busybox
+endef
+endif
+
 define BUSYBOX_INSTALL_LOGGING_SCRIPT
 	if grep -q CONFIG_SYSLOGD=y $(@D)/.config; then \
 		$(INSTALL) -m 0755 -D package/busybox/S01logging \
@@ -199,6 +238,7 @@  define BUSYBOX_KCONFIG_FIXUP_CMDS
 	$(BUSYBOX_INTERNAL_SHADOW_PASSWORDS)
 	$(BUSYBOX_SET_INIT)
 	$(BUSYBOX_SET_WATCHDOG)
+	$(BUSYBOX_CONFIGURE_INDIVIDUAL_BINARIES)
 endef
 
 define BUSYBOX_CONFIGURE_CMDS
@@ -221,6 +261,7 @@  define BUSYBOX_INSTALL_INIT_SYSV
 	$(BUSYBOX_INSTALL_LOGGING_SCRIPT)
 	$(BUSYBOX_INSTALL_WATCHDOG_SCRIPT)
 	$(BUSYBOX_INSTALL_TELNET_SCRIPT)
+	$(BUSYBOX_INSTALL_INDIVIDUAL_BINARIES)
 endef
 
 # Checks to give errors that the user can understand