[Buildroot] [PATCH 0/3] fs: fix and better handle capabilities

All of lore.kernel.org
 help / color / mirror / Atom feed

* [Buildroot] [PATCH 0/3] fs: fix and better handle capabilities
@ 2018-10-27  7:45 Yann E. MORIN
  2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
                   ` (2 more replies)
  0 siblings, 3 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-10-27  7:45 UTC (permalink / raw)
  To: buildroot

Hello All!

As reported by Ricardo in #11216, and recently noticed thanks to the
runtime tests he added, handling file capabilities is broken.

Ricardo did a very good job at pinpointing the issue, and that is caused
by the recent-ish split of the filesystem infra with the use of the
intermediate tarball.

It turns out that playing with fakeroot, tar, and capabilities is a lost
game, as fakeroot behaves badly with the special handling tar does with
the security.capability extended attribute.

To fix that, we postpone handling of capabilities later, down into each
filesystem commands, right after extracting the intermediate tarball.

Discussion about this at the developers days lead to the suggestion
that, maybe, we should in fact not use an intermediate tarball, and
instead have each filesystem duplicate the currently common actions.

This is a bigger endeavour, and one that needs more thinking into.
In the meantime, this patchset is a pragmatic approach to solve the
problem.

Regards,
Yann E. MORIN.

The following changes since commit cbf62fc5692cc04a2f721260d5e7f8a2558b4bb1

  mysql: properly order "depends on" vs. bool (2018-10-26 21:28:31 +0200)

are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to 99d38f609529976ec574c3a05b6665cf3dd0669d

  fs: fix condition to create static devices (2018-10-27 09:43:55 +0200)

----------------------------------------------------------------
Yann E. MORIN (3):
      fs: apply permissions late
      fs: be oblivious of pre-existing xattrs
      fs: fix condition to create static devices

 fs/common.mk | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-27  7:45 [Buildroot] [PATCH 0/3] fs: fix and better handle capabilities Yann E. MORIN
@ 2018-10-27  7:45 ` Yann E. MORIN
  2018-10-27 13:14   ` Matthew Weber
                     ` (3 more replies)
  2018-10-27  7:45 ` [Buildroot] [PATCH 2/3] fs: be oblivious of pre-existing xattrs Yann E. MORIN
  2018-10-27  7:46 ` [Buildroot] [PATCH 3/3] fs: fix condition to create static devices Yann E. MORIN
  2 siblings, 4 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-10-27  7:45 UTC (permalink / raw)
  To: buildroot

The combination of fakeroot, tar, and capabilities is broken, because
fakeroot currently badly handles capabilities, which are currently
simply ignored.

As described in #11216, asking tar to explicitly store and restore
capabilities ends up with a failling build, when tar actually tries to
restore the capabilities. Adding support for capabilities to fakeroot
(by adding host-libcap as dependency) does not fix the problem.

Capabilities are stored in the extended attribute security.capabilty.
It turns out that tar does have special handling when extracting and
restoring that extended attribute, and that fails miserably when running
under fakeroot...

We fix that by offloading the permissions handling down to individual
filesystems.

This needs a split of the makedevs call, with the current and first one
now only responsible for creating the pseudo devices, while the new,
second call does only set the permissions.

Fixes: #11216

This changes the order of steps, and post-fakeroot scripts are now
called before the permissions are set. This could mean breaking existing
setups, but more probably, this woudl sovle some, where files created in
post-fakeroot scripts can now see their permissions appropriately set.

This also slightly breaks the idea behind the intermediate image, which
was supposed to gather all actions common to all filesystems, so that
they are not repeated. Still, most actions are still created only once,
and moving just this is purely a practical and pragmatic workaround.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Matthew Weber <matthew.weber@rockwellcollins.com>
---
 fs/common.mk | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index 453da6010a..569e5d60c5 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -29,8 +29,8 @@
 
 FS_DIR = $(BUILD_DIR)/buildroot-fs
 FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
-ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
-	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
+ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
+ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
 USERS_TABLE = $(FS_DIR)/users_table.txt
 ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
 
@@ -81,14 +81,13 @@ ifneq ($(ROOTFS_USERS_TABLES),)
 	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
 endif
 	PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
-ifneq ($(ROOTFS_DEVICE_TABLES),)
-	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
+ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
+	cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
 ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
 	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
 endif
-endif
-	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
 	echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
+endif
 	$(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
 		echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
 		echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
@@ -108,6 +107,7 @@ define inner-rootfs
 
 ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
 ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
+ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt
 
 ROOTFS_$(2)_DEPENDENCIES += rootfs-common
 
@@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
 	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
 	echo "set -e" >> $$(FAKEROOT_SCRIPT)
 	$$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
+ifneq ($$(ROOTFS_PERMISSION_TABLES),)
+	cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE)
+endif
+	$$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE)
+	echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
 	$$(foreach hook,$$(ROOTFS_$(2)_PRE_GEN_HOOKS),\
 		$$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT)$$(sep))
 	$$(call PRINTF,$$(ROOTFS_REPRODUCIBLE)) >> $$(FAKEROOT_SCRIPT)
-- 
2.14.1

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
@ 2018-10-27 13:14   ` Matthew Weber
  2018-10-30 20:23     ` Yann E. MORIN
  2018-11-03 13:38   ` Thomas Petazzoni
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 25+ messages in thread
From: Matthew Weber @ 2018-10-27 13:14 UTC (permalink / raw)
  To: buildroot

Yann,

On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> The combination of fakeroot, tar, and capabilities is broken, because
> fakeroot currently badly handles capabilities, which are currently
> simply ignored.
>
> As described in #11216, asking tar to explicitly store and restore
> capabilities ends up with a failling build, when tar actually tries to
failling -> failing

> restore the capabilities. Adding support for capabilities to fakeroot
> (by adding host-libcap as dependency) does not fix the problem.
>
> Capabilities are stored in the extended attribute security.capabilty.
Capabilities are stored in the extended attribute security capability.

> It turns out that tar does have special handling when extracting and
> restoring that extended attribute, and that fails miserably when running
> under fakeroot...
>
> We fix that by offloading the permissions handling down to individual
> filesystems.
>
> This needs a split of the makedevs call, with the current and first one
> now only responsible for creating the pseudo devices, while the new,
> second call does only set the permissions.
>
> Fixes: #11216
>
> This changes the order of steps, and post-fakeroot scripts are now
> called before the permissions are set. This could mean breaking existing
> setups, but more probably, this woudl sovle some, where files created in
setups, but more probably, this would solve some, where files created in

> post-fakeroot scripts can now see their permissions appropriately set.
>
> This also slightly breaks the idea behind the intermediate image, which
> was supposed to gather all actions common to all filesystems, so that
> they are not repeated. Still, most actions are still created only once,
> and moving just this is purely a practical and pragmatic workaround.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Matthew Weber <matthew.weber@rockwellcollins.com>

Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>

> ---
>  fs/common.mk | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/common.mk b/fs/common.mk
> index 453da6010a..569e5d60c5 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -29,8 +29,8 @@
>
>  FS_DIR = $(BUILD_DIR)/buildroot-fs
>  FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
> -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
> -       $(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
> +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
>  USERS_TABLE = $(FS_DIR)/users_table.txt
>  ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
>
> @@ -81,14 +81,13 @@ ifneq ($(ROOTFS_USERS_TABLES),)
>         cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
>  endif
>         PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
> -ifneq ($(ROOTFS_DEVICE_TABLES),)
> -       cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> +       cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
>  ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
>         $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
>  endif
> -endif
> -       $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
>         echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
> +endif
>         $(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
>                 echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
>                 echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
> @@ -108,6 +107,7 @@ define inner-rootfs
>
>  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
>  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
> +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt
>
>  ROOTFS_$(2)_DEPENDENCIES += rootfs-common
>
> @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
>         echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
>         echo "set -e" >> $$(FAKEROOT_SCRIPT)
>         $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
> +ifneq ($$(ROOTFS_PERMISSION_TABLES),)
> +       cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE)
> +endif
> +       $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE)

If a package duplicates an entry and is below a user provided rootfs
permissions table similar item, I assume makedev uses the last entry
as the one to set?  If so, should the two lines above be flipped so
the "user provided" can always fixup/override the package default?

Matt

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-27 13:14   ` Matthew Weber
@ 2018-10-30 20:23     ` Yann E. MORIN
  2018-10-31  1:18       ` Matthew Weber
  0 siblings, 1 reply; 25+ messages in thread
From: Yann E. MORIN @ 2018-10-30 20:23 UTC (permalink / raw)
  To: buildroot

Matt, All,

On 2018-10-27 08:14 -0500, Matthew Weber spake thusly:
> On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >
> > The combination of fakeroot, tar, and capabilities is broken, because
> > fakeroot currently badly handles capabilities, which are currently
> > simply ignored.
> >
> > As described in #11216, asking tar to explicitly store and restore
> > capabilities ends up with a failling build, when tar actually tries to
> failling -> failing
> 
> > restore the capabilities. Adding support for capabilities to fakeroot
> > (by adding host-libcap as dependency) does not fix the problem.
> >
> > Capabilities are stored in the extended attribute security.capabilty.
> Capabilities are stored in the extended attribute security capability.

Thanks for the fixes! :-)

Yet, the extended attribute is really named "security.capabilty" (i.e.
with a dot in-between the two words): https://linux.die.net/man/7/capabilities

    Since kernel 2.6.24, the kernel supports associating capability sets
    [...] stored in an extended attribute (see setxattr(2)) named
    security.capability.o

Regards,
Yann E. MORIN.

> > It turns out that tar does have special handling when extracting and
> > restoring that extended attribute, and that fails miserably when running
> > under fakeroot...
> >
> > We fix that by offloading the permissions handling down to individual
> > filesystems.
> >
> > This needs a split of the makedevs call, with the current and first one
> > now only responsible for creating the pseudo devices, while the new,
> > second call does only set the permissions.
> >
> > Fixes: #11216
> >
> > This changes the order of steps, and post-fakeroot scripts are now
> > called before the permissions are set. This could mean breaking existing
> > setups, but more probably, this woudl sovle some, where files created in
> setups, but more probably, this would solve some, where files created in
> 
> > post-fakeroot scripts can now see their permissions appropriately set.
> >
> > This also slightly breaks the idea behind the intermediate image, which
> > was supposed to gather all actions common to all filesystems, so that
> > they are not repeated. Still, most actions are still created only once,
> > and moving just this is purely a practical and pragmatic workaround.
> >
> > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > Cc: Matthew Weber <matthew.weber@rockwellcollins.com>
> 
> Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> 
> > ---
> >  fs/common.mk | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/common.mk b/fs/common.mk
> > index 453da6010a..569e5d60c5 100644
> > --- a/fs/common.mk
> > +++ b/fs/common.mk
> > @@ -29,8 +29,8 @@
> >
> >  FS_DIR = $(BUILD_DIR)/buildroot-fs
> >  FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
> > -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
> > -       $(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
> > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> >  USERS_TABLE = $(FS_DIR)/users_table.txt
> >  ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
> >
> > @@ -81,14 +81,13 @@ ifneq ($(ROOTFS_USERS_TABLES),)
> >         cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
> >  endif
> >         PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
> > -ifneq ($(ROOTFS_DEVICE_TABLES),)
> > -       cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> > +       cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> >  ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
> >         $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
> >  endif
> > -endif
> > -       $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
> >         echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
> > +endif
> >         $(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
> >                 echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
> >                 echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
> > @@ -108,6 +107,7 @@ define inner-rootfs
> >
> >  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
> >  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
> > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt
> >
> >  ROOTFS_$(2)_DEPENDENCIES += rootfs-common
> >
> > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
> >         echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> >         echo "set -e" >> $$(FAKEROOT_SCRIPT)
> >         $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
> > +ifneq ($$(ROOTFS_PERMISSION_TABLES),)
> > +       cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE)
> > +endif
> > +       $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE)
> 
> If a package duplicates an entry and is below a user provided rootfs
> permissions table similar item, I assume makedev uses the last entry
> as the one to set?  If so, should the two lines above be flipped so
> the "user provided" can always fixup/override the package default?
> 
> Matt

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-30 20:23     ` Yann E. MORIN
@ 2018-10-31  1:18       ` Matthew Weber
  2018-10-31 21:49         ` Yann E. MORIN
  0 siblings, 1 reply; 25+ messages in thread
From: Matthew Weber @ 2018-10-31  1:18 UTC (permalink / raw)
  To: buildroot

Yann,


On Tue, Oct 30, 2018 at 3:23 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Matt, All,
>
> On 2018-10-27 08:14 -0500, Matthew Weber spake thusly:
> > On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > >
> > > The combination of fakeroot, tar, and capabilities is broken, because
> > > fakeroot currently badly handles capabilities, which are currently
> > > simply ignored.
> > >
> > > As described in #11216, asking tar to explicitly store and restore
> > > capabilities ends up with a failling build, when tar actually tries to
> > failling -> failing
> >
> > > restore the capabilities. Adding support for capabilities to fakeroot
> > > (by adding host-libcap as dependency) does not fix the problem.
> > >
> > > Capabilities are stored in the extended attribute security.capabilty.
> > Capabilities are stored in the extended attribute security capability.
>
> Thanks for the fixes! :-)
>
> Yet, the extended attribute is really named "security.capabilty" (i.e.
> with a dot in-between the two words): https://linux.die.net/man/7/capabilities
>
>     Since kernel 2.6.24, the kernel supports associating capability sets
>     [...] stored in an extended attribute (see setxattr(2)) named
>     security.capability.o

:-) Fair,  security.capabilty -> security.capability

>
> Regards,
> Yann E. MORIN.
>
> > > It turns out that tar does have special handling when extracting and
> > > restoring that extended attribute, and that fails miserably when running
> > > under fakeroot...
> > >
> > > We fix that by offloading the permissions handling down to individual
> > > filesystems.
> > >
> > > This needs a split of the makedevs call, with the current and first one
> > > now only responsible for creating the pseudo devices, while the new,
> > > second call does only set the permissions.
> > >
> > > Fixes: #11216
> > >
> > > This changes the order of steps, and post-fakeroot scripts are now
> > > called before the permissions are set. This could mean breaking existing
> > > setups, but more probably, this woudl sovle some, where files created in
> > setups, but more probably, this would solve some, where files created in
> >
> > > post-fakeroot scripts can now see their permissions appropriately set.
> > >
> > > This also slightly breaks the idea behind the intermediate image, which
> > > was supposed to gather all actions common to all filesystems, so that
> > > they are not repeated. Still, most actions are still created only once,
> > > and moving just this is purely a practical and pragmatic workaround.
> > >
> > > Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> > > Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > > Cc: Matthew Weber <matthew.weber@rockwellcollins.com>
> >
> > Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>
> >
> > > ---
> > >  fs/common.mk | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/common.mk b/fs/common.mk
> > > index 453da6010a..569e5d60c5 100644
> > > --- a/fs/common.mk
> > > +++ b/fs/common.mk
> > > @@ -29,8 +29,8 @@
> > >
> > >  FS_DIR = $(BUILD_DIR)/buildroot-fs
> > >  FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
> > > -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
> > > -       $(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> > > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
> > > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> > >  USERS_TABLE = $(FS_DIR)/users_table.txt
> > >  ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
> > >
> > > @@ -81,14 +81,13 @@ ifneq ($(ROOTFS_USERS_TABLES),)
> > >         cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
> > >  endif
> > >         PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
> > > -ifneq ($(ROOTFS_DEVICE_TABLES),)
> > > -       cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> > > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> > > +       cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> > >  ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
> > >         $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
> > >  endif
> > > -endif
> > > -       $(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
> > >         echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
> > > +endif
> > >         $(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
> > >                 echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
> > >                 echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
> > > @@ -108,6 +107,7 @@ define inner-rootfs
> > >
> > >  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
> > >  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
> > > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt
> > >
> > >  ROOTFS_$(2)_DEPENDENCIES += rootfs-common
> > >
> > > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
> > >         echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> > >         echo "set -e" >> $$(FAKEROOT_SCRIPT)
> > >         $$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
> > > +ifneq ($$(ROOTFS_PERMISSION_TABLES),)
> > > +       cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE)
> > > +endif
> > > +       $$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE)
> >
> > If a package duplicates an entry and is below a user provided rootfs
> > permissions table similar item, I assume makedev uses the last entry
> > as the one to set?  If so, should the two lines above be flipped so
> > the "user provided" can always fixup/override the package default?
> >
> > Matt
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'



-- 
Matthew L Weber / Pr Software Engineer
Airborne Information Systems / RC Linux Secure Platforms
MS 131-100, C Ave NE, Cedar Rapids, IA, 52498, USA
www.rockwellcollins.com

Note: Any Export License Required Information and License Restricted
Third Party Intellectual Property (TPIP) content must be encrypted and
sent to matthew.weber at corp.rockwellcollins.com.

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-31  1:18       ` Matthew Weber
@ 2018-10-31 21:49         ` Yann E. MORIN
  2018-11-02 20:29           ` Matthew Weber
  0 siblings, 1 reply; 25+ messages in thread
From: Yann E. MORIN @ 2018-10-31 21:49 UTC (permalink / raw)
  To: buildroot

Matt, All,

On 2018-10-30 20:18 -0500, Matthew Weber spake thusly:
> On Tue, Oct 30, 2018 at 3:23 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> >     Since kernel 2.6.24, the kernel supports associating capability sets
> >     [...] stored in an extended attribute (see setxattr(2)) named
> >     security.capability.o
> :-) Fair,  security.capabilty -> security.capability

Muahaha! Thanks :-)

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-31 21:49         ` Yann E. MORIN
@ 2018-11-02 20:29           ` Matthew Weber
  0 siblings, 0 replies; 25+ messages in thread
From: Matthew Weber @ 2018-11-02 20:29 UTC (permalink / raw)
  To: buildroot

Yann,


On Wed, Oct 31, 2018 at 4:49 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Matt, All,
>
> On 2018-10-30 20:18 -0500, Matthew Weber spake thusly:
> > On Tue, Oct 30, 2018 at 3:23 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> > >     Since kernel 2.6.24, the kernel supports associating capability sets
> > >     [...] stored in an extended attribute (see setxattr(2)) named
> > >     security.capability.o
> > :-) Fair,  security.capabilty -> security.capability
>

Tested-by: Matthew Weber <matthew.weber@rockwellcollins.com>

Ran some scenaios as a regression on existing target builds before and
after the change.

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
  2018-10-27 13:14   ` Matthew Weber
@ 2018-11-03 13:38   ` Thomas Petazzoni
  2018-11-10 17:17     ` Yann E. MORIN
  2018-11-07 23:43   ` Arnout Vandecappelle
  2018-11-08 22:58   ` Peter Korsgaard
  3 siblings, 1 reply; 25+ messages in thread
From: Thomas Petazzoni @ 2018-11-03 13:38 UTC (permalink / raw)
  To: buildroot

Hello,

On Sat, 27 Oct 2018 09:45:58 +0200, Yann E. MORIN wrote:

> This needs a split of the makedevs call, with the current and first one
> now only responsible for creating the pseudo devices, while the new,
> second call does only set the permissions.

What happens if there are special permissions/extended attributes set
on pseudo devices in the static device table ?

> diff --git a/fs/common.mk b/fs/common.mk
> index 453da6010a..569e5d60c5 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -29,8 +29,8 @@
>  
>  FS_DIR = $(BUILD_DIR)/buildroot-fs
>  FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt

I no longer like the name FULL_DEVICE_TABLE, nor device_table.txt,
because it's no longer the "full device table".

> -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
> -	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
> +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
>  USERS_TABLE = $(FS_DIR)/users_table.txt
>  ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
>  
> @@ -81,14 +81,13 @@ ifneq ($(ROOTFS_USERS_TABLES),)
>  	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
>  endif
>  	PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
> -ifneq ($(ROOTFS_DEVICE_TABLES),)
> -	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> +	cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
>  ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
>  	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
>  endif
> -endif
> -	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
>  	echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
> +endif
>  	$(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
>  		echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
>  		echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
> @@ -108,6 +107,7 @@ define inner-rootfs
>  
>  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
>  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
> +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt

There is no reason for this variable to be filesystem-specific, nor for
this permissions.txt file to be generated for each filesystem format.

So I'd prefer to see something like this in fs/common.mk:

ROOTFS_FULL_STATIC_DEVICE_TABLE = $(FS_DIR)/full_static_device_table.txt
ROOTFS_FULL_PERMISSION_TABLE = $(FS_DIR)/full_permission_table.txt

both are generated in fs/common.mk, but only
ROOTFS_FULL_STATIC_DEVICE_TABLE is used in the makedevs call in
fs/common.mk.


>  ROOTFS_$(2)_DEPENDENCIES += rootfs-common
>  
> @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
>  	echo "set -e" >> $$(FAKEROOT_SCRIPT)
>  	$$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
> +ifneq ($$(ROOTFS_PERMISSION_TABLES),)
> +	cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE)
> +endif
> +	$$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE)

i.e, those lines would migrate back into fs/common.mk.

> +	echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)

And this would use $(ROOTFS_FULL_PERMISSION_TABLE)

This would make the whole thing more consistent IMO. Of course unless
we decide that pseudo devices in the static device table can have
extended attributes, in which case, the makedevs for static devices
anyway needs to be moved to the per-filesystem code.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-03 13:38   ` Thomas Petazzoni
@ 2018-11-10 17:17     ` Yann E. MORIN
  0 siblings, 0 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-11-10 17:17 UTC (permalink / raw)
  To: buildroot

Thomas, All,

On 2018-11-03 14:38 +0100, Thomas Petazzoni spake thusly:
> On Sat, 27 Oct 2018 09:45:58 +0200, Yann E. MORIN wrote:
> 
> > This needs a split of the makedevs call, with the current and first one
> > now only responsible for creating the pseudo devices, while the new,
> > second call does only set the permissions.
> 
> What happens if there are special permissions/extended attributes set
> on pseudo devices in the static device table ?

This is of no consequence, because that is not possible to do with our
makedevs tool: it only ever supports setting capabilties; even though we
claim xattr, it's really capabilties:

    https://git.buildroot.org/buildroot/tree/package/makedevs/makedevs.c#n355

Capabilities are 100% useless on device nodes, because they are only
used for read/write/ioctl. capabilties are used to give a process more
permissions than it would otherwise have, i.e. capabilities are to be
set on executables.

So it does not make sense to set capabilties on device nodes in /dev...

> > diff --git a/fs/common.mk b/fs/common.mk
> > index 453da6010a..569e5d60c5 100644
> > --- a/fs/common.mk
> > +++ b/fs/common.mk
> > @@ -29,8 +29,8 @@
> >  
> >  FS_DIR = $(BUILD_DIR)/buildroot-fs
> >  FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
> 
> I no longer like the name FULL_DEVICE_TABLE, nor device_table.txt,
> because it's no longer the "full device table".

Well, now at least, it only contains devices, and not permissions. And
yes, it is the full device table now, because it does contain all the
_devices_ that are to be created: those from the static table(S), and
those from packages, and nothing more.

One could argue that the previous denomination was in fact wrong, and
that it is now correct. ;-)

However, see below...

> > -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
> > -	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> > +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
> > +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> >  USERS_TABLE = $(FS_DIR)/users_table.txt
> >  ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
> >  
> > @@ -81,14 +81,13 @@ ifneq ($(ROOTFS_USERS_TABLES),)
> >  	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
> >  endif
> >  	PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
> > -ifneq ($(ROOTFS_DEVICE_TABLES),)
> > -	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> > +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> > +	cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> >  ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
> >  	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
> >  endif
> > -endif
> > -	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
> >  	echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
> > +endif
> >  	$(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
> >  		echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
> >  		echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
> > @@ -108,6 +107,7 @@ define inner-rootfs
> >  
> >  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
> >  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
> > +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt
> 
> There is no reason for this variable to be filesystem-specific, nor for
> this permissions.txt file to be generated for each filesystem format.
> 
> So I'd prefer to see something like this in fs/common.mk:
> 
> ROOTFS_FULL_STATIC_DEVICE_TABLE = $(FS_DIR)/full_static_device_table.txt
> ROOTFS_FULL_PERMISSION_TABLE = $(FS_DIR)/full_permission_table.txt
> 
> both are generated in fs/common.mk, but only
> ROOTFS_FULL_STATIC_DEVICE_TABLE is used in the makedevs call in
> fs/common.mk.

ACK.

> >  ROOTFS_$(2)_DEPENDENCIES += rootfs-common
> >  
> > @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
> >  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
> >  	echo "set -e" >> $$(FAKEROOT_SCRIPT)
> >  	$$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
> > +ifneq ($$(ROOTFS_PERMISSION_TABLES),)
> > +	cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE)
> > +endif
> > +	$$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE)
> 
> i.e, those lines would migrate back into fs/common.mk.
> 
> > +	echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
> 
> And this would use $(ROOTFS_FULL_PERMISSION_TABLE)

ACK

> This would make the whole thing more consistent IMO. Of course unless
> we decide that pseudo devices in the static device table can have
> extended attributes, in which case, the makedevs for static devices
> anyway needs to be moved to the per-filesystem code.

As I explained above, I believe it does not make sense.

However, as we previously discussed, we maybe should drop the
inter,mediate rootfs step, and revert to duplicating everything in each
filesystem steps, each starting off by copying the origian target/ to
their own copy and hack that.

I'm not sure which is the simplest to do to be ready for this release
cycle, I'll be investigating this more by the end of the WE...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
  2018-10-27 13:14   ` Matthew Weber
  2018-11-03 13:38   ` Thomas Petazzoni
@ 2018-11-07 23:43   ` Arnout Vandecappelle
  2018-11-09 20:13     ` Arnout Vandecappelle
  2018-11-10 17:57     ` Yann E. MORIN
  2018-11-08 22:58   ` Peter Korsgaard
  3 siblings, 2 replies; 25+ messages in thread
From: Arnout Vandecappelle @ 2018-11-07 23:43 UTC (permalink / raw)
  To: buildroot



On 27/10/18 09:45, Yann E. MORIN wrote:
> The combination of fakeroot, tar, and capabilities is broken, because
> fakeroot currently badly handles capabilities, which are currently
> simply ignored.

 "simply ignored" can't be the case, otherwise it still wouldn't work after this
patch.

 I *think* the real problem is that fakeroot forgets to wrap the setxattrat and
lsetxattrat syscalls, and these are the ones that are used by tar.

> As described in #11216, asking tar to explicitly store and restore
> capabilities ends up with a failling build, when tar actually tries to
> restore the capabilities. Adding support for capabilities to fakeroot
> (by adding host-libcap as dependency) does not fix the problem.

 Just to be clear: with this patch, the 'tar' filesystem does still work because
the creation of the tarball works OK, it's just the extraction that goes wrong,
right?


> Capabilities are stored in the extended attribute security.capabilty.
> It turns out that tar does have special handling when extracting and
> restoring that extended attribute, and that fails miserably when running
> under fakeroot...
> 
> We fix that by offloading the permissions handling down to individual
> filesystems.
> 
> This needs a split of the makedevs call, with the current and first one
> now only responsible for creating the pseudo devices, while the new,
> second call does only set the permissions.

 Why? Is that just to reduce the impact on post-fakeroot scripts?

 I'd rather move the entire makedevs call to the per-filesystem phase.

> 
> Fixes: #11216
> 
> This changes the order of steps, and post-fakeroot scripts are now
> called before the permissions are set. This could mean breaking existing
> setups, but more probably, this woudl sovle some, where files created in
> post-fakeroot scripts can now see their permissions appropriately set.

 Since extracting xattrs doesn't work correctly, this is not true. And normally
in a post-fakeroot script, you would do any permission setting in the script
itself and not use the permissions table. So I think this statement is utterly
wrong. That said, I doubt that post-build scripts would be affected by the
permissions tables anyway.


> This also slightly breaks the idea behind the intermediate image, which
> was supposed to gather all actions common to all filesystems, so that
> they are not repeated. Still, most actions are still created only once,
> and moving just this is purely a practical and pragmatic workaround.

 Still, I'd prefer to fix fakeroot :-)

> 
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Matthew Weber <matthew.weber@rockwellcollins.com>
> ---
>  fs/common.mk | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/common.mk b/fs/common.mk
> index 453da6010a..569e5d60c5 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -29,8 +29,8 @@
>  
>  FS_DIR = $(BUILD_DIR)/buildroot-fs
>  FULL_DEVICE_TABLE = $(FS_DIR)/device_table.txt
> -ROOTFS_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE) \
> -	$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
> +ROOTFS_PERMISSION_TABLES = $(call qstrip,$(BR2_ROOTFS_DEVICE_TABLE))
> +ROOTFS_STATIC_DEVICE_TABLES = $(call qstrip,$(BR2_ROOTFS_STATIC_DEVICE_TABLE))
>  USERS_TABLE = $(FS_DIR)/users_table.txt
>  ROOTFS_USERS_TABLES = $(call qstrip,$(BR2_ROOTFS_USERS_TABLES))
>  
> @@ -81,14 +81,13 @@ ifneq ($(ROOTFS_USERS_TABLES),)
>  	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
>  endif
>  	PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
> -ifneq ($(ROOTFS_DEVICE_TABLES),)
> -	cat $(ROOTFS_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> +ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
> +	cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
>  ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
>  	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
>  endif
> -endif
> -	$(call PRINTF,$(PACKAGES_PERMISSIONS_TABLE)) >> $(FULL_DEVICE_TABLE)
>  	echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
> +endif
>  	$(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
>  		echo "echo '$(TERM_BOLD)>>>   Executing fakeroot script $(s)$(TERM_RESET)'" >> $(FAKEROOT_SCRIPT); \
>  		echo $(EXTRA_ENV) $(s) $(TARGET_DIR) $(BR2_ROOTFS_POST_SCRIPT_ARGS) >> $(FAKEROOT_SCRIPT)$(sep))
> @@ -108,6 +107,7 @@ define inner-rootfs
>  
>  ROOTFS_$(2)_DIR = $$(FS_DIR)/$(1)
>  ROOTFS_$(2)_TARGET_DIR = $$(ROOTFS_$(2)_DIR)/target
> +ROOTFS_$(2)_PERMISSION_TABLE = $$(ROOTFS_$(2)_DIR)/permissions.txt

 BTW I agree with Thomas that it's better to create this file only once and only
do the makedevs call in the per-filesystem stage.

 Regards,
 Arnout


>  
>  ROOTFS_$(2)_DEPENDENCIES += rootfs-common
>  
> @@ -149,6 +149,11 @@ $$(BINARIES_DIR)/rootfs.$(1): $$(ROOTFS_$(2)_DEPENDENCIES)
>  	echo '#!/bin/sh' > $$(FAKEROOT_SCRIPT)
>  	echo "set -e" >> $$(FAKEROOT_SCRIPT)
>  	$$(call PRINTF,$$(ROOTFS_COMMON_UNTAR_CMD)) >> $$(FAKEROOT_SCRIPT)
> +ifneq ($$(ROOTFS_PERMISSION_TABLES),)
> +	cat $$(ROOTFS_PERMISSION_TABLES) > $$(ROOTFS_$(2)_PERMISSION_TABLE)
> +endif
> +	$$(call PRINTF,$$(PACKAGES_PERMISSIONS_TABLE)) >> $$(ROOTFS_$(2)_PERMISSION_TABLE)
> +	echo "$$(HOST_DIR)/bin/makedevs -d $$(ROOTFS_$(2)_PERMISSION_TABLE) $$(TARGET_DIR)" >> $$(FAKEROOT_SCRIPT)
>  	$$(foreach hook,$$(ROOTFS_$(2)_PRE_GEN_HOOKS),\
>  		$$(call PRINTF,$$($$(hook))) >> $$(FAKEROOT_SCRIPT)$$(sep))
>  	$$(call PRINTF,$$(ROOTFS_REPRODUCIBLE)) >> $$(FAKEROOT_SCRIPT)
> 

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-07 23:43   ` Arnout Vandecappelle
@ 2018-11-09 20:13     ` Arnout Vandecappelle
  2018-11-10 17:08       ` Yann E. MORIN
  2018-11-10 17:57     ` Yann E. MORIN
  1 sibling, 1 reply; 25+ messages in thread
From: Arnout Vandecappelle @ 2018-11-09 20:13 UTC (permalink / raw)
  To: buildroot


On 08/11/2018 00:43, Arnout Vandecappelle wrote:
>  I *think* the real problem is that fakeroot forgets to wrap the setxattrat and
> lsetxattrat syscalls, and these are the ones that are used by tar.

?So this was a stupid mistake - there is no setxattrat syscall, it's a function
that is defined by tar itself.

?Peter's analysis looks a lot more accurate.


?Regards,
?Arnout

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-09 20:13     ` Arnout Vandecappelle
@ 2018-11-10 17:08       ` Yann E. MORIN
  0 siblings, 0 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-11-10 17:08 UTC (permalink / raw)
  To: buildroot

Arnout, All,

On 2018-11-09 21:13 +0100, Arnout Vandecappelle spake thusly:
> On 08/11/2018 00:43, Arnout Vandecappelle wrote:
> >  I *think* the real problem is that fakeroot forgets to wrap the setxattrat and
> > lsetxattrat syscalls, and these are the ones that are used by tar.
> 
> ?So this was a stupid mistake - there is no setxattrat syscall, it's a function
> that is defined by tar itself.

ACK, thanks for the feedback.

> ?Peter's analysis looks a lot more accurate.

Except he was not using fakeroot, but sudo, which does work (as
explained in BZ-11216).

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-07 23:43   ` Arnout Vandecappelle
  2018-11-09 20:13     ` Arnout Vandecappelle
@ 2018-11-10 17:57     ` Yann E. MORIN
  2018-11-11 16:02       ` Arnout Vandecappelle
  2018-11-11 20:02       ` Peter Korsgaard
  1 sibling, 2 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-11-10 17:57 UTC (permalink / raw)
  To: buildroot

Arnout, All,

On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly:
> On 27/10/18 09:45, Yann E. MORIN wrote:
> > The combination of fakeroot, tar, and capabilities is broken, because
> > fakeroot currently badly handles capabilities, which are currently
> > simply ignored.
> 
>  "simply ignored" can't be the case, otherwise it still wouldn't work after this
> patch.

No, because fakeroot really badly handles capabilties, as demonstrated
in another mail in that thread. And yes, capabilties *are* currently
simply ignored, and that's on purpose (not that changing would fix it
anyway, and I'm not even totally sure this is related, and neither am I
really sure to understand what it even really means), added in
c994c3db1f7d (fakeroot: disable capabilities):

    https://git.buildroot.org/buildroot/tree/package/fakeroot/fakeroot.mk#n12

    # Force capabilities detection off
    # For now these are process capabilities (faked) rather than file
    # so they're of no real use
    HOST_FAKEROOT_CONF_ENV = \
        ac_cv_header_sys_capability_h=no \
        ac_cv_func_capset=no

[--SNIP--]
> > As described in #11216, asking tar to explicitly store and restore
> > capabilities ends up with a failling build, when tar actually tries to
> > restore the capabilities. Adding support for capabilities to fakeroot
> > (by adding host-libcap as dependency) does not fix the problem.
> 
>  Just to be clear: with this patch, the 'tar' filesystem does still work because
> the creation of the tarball works OK, it's just the extraction that goes wrong,
> right?

Oh, you poinpoint an issue: the tar filesystem must be also modified to
add --xattrs-include='*' otherwise it would not contain any xattrs, and
even less so, any capabilty.

So, with this patch, the 'tar' filesystem is not fixed, but this is not
a regression either, as it did not have capabilties, and it still does
not.

Now, should we fix the tar filesystem, a tarball of the rootfs is
supposed to be extracted by root, the real root, not a fake root, and
as Peter demonstrated, there is no problem with capabilities in a
tarball that is extracted by root.

> > Capabilities are stored in the extended attribute security.capabilty.
> > It turns out that tar does have special handling when extracting and
> > restoring that extended attribute, and that fails miserably when running
> > under fakeroot...
> > 
> > We fix that by offloading the permissions handling down to individual
> > filesystems.
> > 
> > This needs a split of the makedevs call, with the current and first one
> > now only responsible for creating the pseudo devices, while the new,
> > second call does only set the permissions.
> 
>  Why? Is that just to reduce the impact on post-fakeroot scripts?

The idea of the common, intermediate tarball was to include all the
common actions, to avoid repeating them for each filesystem.

Since the creating of device nodes in /dev is not impacted by the
capabilties, I left that in the common part, and only moved the
offending part (setting permissions, which includes capabilties) to the
per-filesystem actions.

It had about nothing to do with the post-fakeroot scripts that I had
thought about.

>  I'd rather move the entire makedevs call to the per-filesystem phase.

I'm still not convinced, as this is not needed (capabilties don't make
sense on device nodes), except if we were to entirely drop the
intemediate tarball altogether.

> > Fixes: #11216
> > 
> > This changes the order of steps, and post-fakeroot scripts are now
> > called before the permissions are set. This could mean breaking existing
> > setups, but more probably, this woudl sovle some, where files created in
> > post-fakeroot scripts can now see their permissions appropriately set.
> 
>  Since extracting xattrs doesn't work correctly, this is not true.i

Sorry, re-reading this, I see where you choked on my explanations. The
underlying reasoning I had, was that files created in a post-fakeroot
scripts could have appropriate capabilties by providing a permissions
table, which is _now_ applied after those files are created.

So, before this patch, capabilities created in a post-fakeroot scripts
were not working, and there was no way to set capabilties on those
files. With this patch, they are not working either, but there is a
workaround by providing a permissions table.

> And normally
> in a post-fakeroot script, you would do any permission setting in the script
> itself and not use the permissions table.

Except that was not working so far, and nobody complained, so nobody was
doing that anyway.

> So I think this statement is utterly
> wrong.

s/utterly//  makes it easier to read, thanks. And yes, it was wrong,
except it did not properly convey my thinking. I hope the above
explanations make it a bit less wrong.

> That said, I doubt that post-build scripts would be affected by the
> permissions tables anyway.

> > This also slightly breaks the idea behind the intermediate image, which
> > was supposed to gather all actions common to all filesystems, so that
> > they are not repeated. Still, most actions are still created only once,
> > and moving just this is purely a practical and pragmatic workaround.
> 
>  Still, I'd prefer to fix fakeroot :-)

Then, please be my guest! ;-) I already fixed something in fakeroot, and
I dread working on that code base any more...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-10 17:57     ` Yann E. MORIN
@ 2018-11-11 16:02       ` Arnout Vandecappelle
  2018-11-11 16:44         ` Yann E. MORIN
  2018-11-11 20:03         ` Peter Korsgaard
  2018-11-11 20:02       ` Peter Korsgaard
  1 sibling, 2 replies; 25+ messages in thread
From: Arnout Vandecappelle @ 2018-11-11 16:02 UTC (permalink / raw)
  To: buildroot



On 10/11/2018 18:57, Yann E. MORIN wrote:
> Arnout, All,
> 
> On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly:
[snip]
>>  Still, I'd prefer to fix fakeroot :-)
> 
> Then, please be my guest! ;-) I already fixed something in fakeroot, and
> I dread working on that code base any more...

 Time to switch to pseudo? The .11-rc period is ideal for that, isn't it? :-)

 Regards,
 Arnout

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-11 16:02       ` Arnout Vandecappelle
@ 2018-11-11 16:44         ` Yann E. MORIN
  2018-11-11 20:03         ` Peter Korsgaard
  1 sibling, 0 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-11-11 16:44 UTC (permalink / raw)
  To: buildroot

Arnout, All,

On 2018-11-11 17:02 +0100, Arnout Vandecappelle spake thusly:
> On 10/11/2018 18:57, Yann E. MORIN wrote:
> > Arnout, All,
> > 
> > On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly:
> [snip]
> >>  Still, I'd prefer to fix fakeroot :-)
> > 
> > Then, please be my guest! ;-) I already fixed something in fakeroot, and
> > I dread working on that code base any more...
> 
>  Time to switch to pseudo? The .11-rc period is ideal for that, isn't it? :-)

I already made that joke on IRC the other day! ;-)
    2018-11-09 22:01 < y_morin> Maybe we will eventually have to switch
                                to an alternative (pseudo?)  NOOOO!!!!!! :-]

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-11 16:02       ` Arnout Vandecappelle
  2018-11-11 16:44         ` Yann E. MORIN
@ 2018-11-11 20:03         ` Peter Korsgaard
  1 sibling, 0 replies; 25+ messages in thread
From: Peter Korsgaard @ 2018-11-11 20:03 UTC (permalink / raw)
  To: buildroot

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

 > On 10/11/2018 18:57, Yann E. MORIN wrote:
 >> Arnout, All,
 >> 
 >> On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly:
 > [snip]
 >>> Still, I'd prefer to fix fakeroot :-)
 >> 
 >> Then, please be my guest! ;-) I already fixed something in fakeroot, and
 >> I dread working on that code base any more...

 >  Time to switch to pseudo? The .11-rc period is ideal for that, isn't it? :-)

I see the smiley, but does pseudo correctly handle xattrs/capabilities?

-- 
Bye, Peter Korsgaard

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-10 17:57     ` Yann E. MORIN
  2018-11-11 16:02       ` Arnout Vandecappelle
@ 2018-11-11 20:02       ` Peter Korsgaard
  2018-11-12  8:17         ` Yann E. MORIN
  1 sibling, 1 reply; 25+ messages in thread
From: Peter Korsgaard @ 2018-11-11 20:02 UTC (permalink / raw)
  To: buildroot

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> Just to be clear: with this patch, the 'tar' filesystem does still work because
 >> the creation of the tarball works OK, it's just the extraction that goes wrong,
 >> right?

 > Oh, you poinpoint an issue: the tar filesystem must be also modified to
 > add --xattrs-include='*' otherwise it would not contain any xattrs, and
 > even less so, any capabilty.

I believe my tests showed that --xattrs-include isn't needed when
creating the tarball (but _IS_ needed when extracting), as long as
--xattrs is passed.

-- 
Bye, Peter Korsgaard

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-11 20:02       ` Peter Korsgaard
@ 2018-11-12  8:17         ` Yann E. MORIN
  0 siblings, 0 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-11-12  8:17 UTC (permalink / raw)
  To: buildroot

On 2018-11-11 21:02 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
>  > Oh, you poinpoint an issue: the tar filesystem must be also modified to
>  > add --xattrs-include='*' otherwise it would not contain any xattrs, and
>  > even less so, any capabilty.
> 
> I believe my tests showed that --xattrs-include isn't needed when
> creating the tarball (but _IS_ needed when extracting), as long as
> --xattrs is passed.

Right, but the tar filesystem does have to be fixed, anyway.

I don't like it that the extraction is not symetric to the creation, and
since this has puzzled us (and a lot of other people), I'd prefer if we
were explicit which xattrs we ionclude.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
                     ` (2 preceding siblings ...)
  2018-11-07 23:43   ` Arnout Vandecappelle
@ 2018-11-08 22:58   ` Peter Korsgaard
  2018-11-09  8:55     ` Peter Korsgaard
  2018-11-10 17:07     ` Yann E. MORIN
  3 siblings, 2 replies; 25+ messages in thread
From: Peter Korsgaard @ 2018-11-08 22:58 UTC (permalink / raw)
  To: buildroot

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > The combination of fakeroot, tar, and capabilities is broken, because
 > fakeroot currently badly handles capabilities, which are currently
 > simply ignored.

 > As described in #11216, asking tar to explicitly store and restore
 > capabilities ends up with a failling build, when tar actually tries to
 > restore the capabilities. Adding support for capabilities to fakeroot
 > (by adding host-libcap as dependency) does not fix the problem.

 > Capabilities are stored in the extended attribute security.capabilty.
 > It turns out that tar does have special handling when extracting and
 > restoring that extended attribute, and that fails miserably when running
 > under fakeroot...

Hmm, playing a bit around with tar here (1.29, Debian) it looks like it
doesn't actually extract the security.capability xattrs when --xattrs is
used unless --xattrs-include='*.*' is used:

getcap /usr/bin/i3status
/usr/bin/i3status = cap_net_admin+ep

sudo tar -cvvvf foo.tar /usr/bin/i3status
tar: Removing leading `/' from member names
-rwxr-xr-x root/root     84888 2017-01-21 15:42 /usr/bin/i3status
hexdump -C foo.tar | grep -A2 ecu

No security.capability in the .tar

sudo tar --xattrs -cvvvf foo.tar /usr/bin/i3status
tar: Removing leading `/' from member names
-rwxr-xr-x  root/root     84888 2017-01-21 15:42 /usr/bin/i3status
hexdump -C foo.tar | grep -A2 ecu
00000240  43 48 49 4c 59 2e 78 61  74 74 72 2e 73 65 63 75  |CHILY.xattr.secu|
00000250  72 69 74 79 2e 63 61 70  61 62 69 6c 69 74 79 3d  |rity.capability=|
00000260  01 00 00 02 00 10 00 00  00 00 00 00 00 00 00 00  |................|

With --xattrs they are included but not listed in the verbose output:

sudo tar --xattrs --xattrs-include='*.*' -cvvvf foo.tar /usr/bin/i3status
tar: Removing leading `/' from member names
-rwxr-xr-x* root/root     84888 2017-01-21 15:42 /usr/bin/i3status
  x: 20 security.capability
hexdump -C foo.tar | grep -A2 ecu
00000240  43 48 49 4c 59 2e 78 61  74 74 72 2e 73 65 63 75  |CHILY.xattr.secu|
00000250  72 69 74 79 2e 63 61 70  61 62 69 6c 69 74 79 3d  |rity.capability=|
00000260  01 00 00 02 00 10 00 00  00 00 00 00 00 00 00 00  |................|

Same .tar content but now also listed in the verbose output.


For extraction:

tar -xvvvf ../foo.tar && getcap usr/bin/i3status
-rwxr-xr-x root/root     84888 2017-01-21 15:42 usr/bin/i3status

No capability.

tar --xattrs -xvvvf ../foo.tar && getcap usr/bin/i3status
-rwxr-xr-x  root/root     84888 2017-01-21 15:42 usr/bin/i3status

Still no capability.

tar --xattrs --xattrs-include='*.*' -xvvvf ../foo.tar && getcap usr/bin/i3status
-rwxr-xr-x* root/root     84888 2017-01-21 15:42 usr/bin/i3status
  x: 20 security.capability
usr/bin/i3status = cap_net_admin+ep

Now it works.

I don't see where we are passing --xattrs-include. Are we sure this is a
fakeroot issue after all?

-- 
Bye, Peter Korsgaard

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-08 22:58   ` Peter Korsgaard
@ 2018-11-09  8:55     ` Peter Korsgaard
  2018-11-10 17:07     ` Yann E. MORIN
  1 sibling, 0 replies; 25+ messages in thread
From: Peter Korsgaard @ 2018-11-09  8:55 UTC (permalink / raw)
  To: buildroot

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

Hi,

 > tar --xattrs --xattrs-include='*.*' -xvvvf ../foo.tar && getcap usr/bin/i3status
 > -rwxr-xr-x* root/root     84888 2017-01-21 15:42 usr/bin/i3status
 >   x: 20 security.capability
 > usr/bin/i3status = cap_net_admin+ep

 > Now it works.

 > I don't see where we are passing --xattrs-include. Are we sure this is a
 > fakeroot issue after all?

Digging further, this is apparently expected (if confusing) behaviour:

https://bugzilla.redhat.com/show_bug.cgi?id=771927#c21

-- 
Bye, Peter Korsgaard

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

* [Buildroot] [PATCH 1/3] fs: apply permissions late
  2018-11-08 22:58   ` Peter Korsgaard
  2018-11-09  8:55     ` Peter Korsgaard
@ 2018-11-10 17:07     ` Yann E. MORIN
  1 sibling, 0 replies; 25+ messages in thread
From: Yann E. MORIN @ 2018-11-10 17:07 UTC (permalink / raw)
  To: buildroot

Peter, All,

On 2018-11-08 23:58 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
> 
>  > The combination of fakeroot, tar, and capabilities is broken, because
>  > fakeroot currently badly handles capabilities, which are currently
>  > simply ignored.
> 
>  > As described in #11216, asking tar to explicitly store and restore
>  > capabilities ends up with a failling build, when tar actually tries to
>  > restore the capabilities. Adding support for capabilities to fakeroot
>  > (by adding host-libcap as dependency) does not fix the problem.
> 
>  > Capabilities are stored in the extended attribute security.capabilty.
>  > It turns out that tar does have special handling when extracting and
>  > restoring that extended attribute, and that fails miserably when running
>  > under fakeroot...
> 
> Hmm, playing a bit around with tar here (1.29, Debian) it looks like it
> doesn't actually extract the security.capability xattrs when --xattrs is
> used unless --xattrs-include='*.*' is used:
> 
> getcap /usr/bin/i3status
> /usr/bin/i3status = cap_net_admin+ep
> 
> sudo tar -cvvvf foo.tar /usr/bin/i3status

Sorry, but as discussed on IRC, your analysis is incorrect, because you
are really root, by mean of sudo. The issue manifests itself only under
fakeroot.

[...]
> I don't see where we are passing --xattrs-include.

We are not, and this is all the grief reported in BZ-11216.

> Are we sure this is a
> fakeroot issue after all?

So, here is a report of a bit more experimentations. 'master' is 6a5e9a7ac6.
The configuration is from support/testing/tests/core/test_file_capabilities.py
The tar that is used is the one from my system: tar (GNU tar) 1.29

    master
        no capability in rootfs

    master
    tar with --xattrs-include='*'
        no capability in rootfs

    master
    untar with --xattrs-include='*'
        no capability in rootfs

    master
    tar with --xattrs-include='*'
    untar with --xattrs-include='*'
        File [...]/usr/sbin/getcap has unrecognised filetype 0, ignoring
        /usr/sbin/getcap is missing from rootfs

    master
    fakeroot with libcap
        no capability in rootfs

    master
    fakeroot with libcap
    tar with --xattrs-include='*'
    untar with --xattrs-include='*'
        File [...]/usr/sbin/getcap has unrecognised filetype 0, ignoring
        /usr/sbin/getcap is missing from rootfs

To be noted: the rootfs in that config is a squashfs. mksquashfs simply
ignores the unknown filetype, so getcap is missing in the filesystem.
However, should one switch to using ext4 for the test, then mkfs.ext4
would choke on that unknown filetype, and error out, which we would
catch (same behaviour whether fakeroot is linked with libcap or not):

    Copying files into the device: __populate_fs: ignoring entry "getcap"
    getcap: File not found by ext2_lookup while looking up "getcap"
    mkfs.ext4: File not found by ext2_lookup while populating file system
    *** Maybe you need to increase the filesystem size (BR2_TARGET_ROOTFS_EXT2_SIZE)
    fs/ext2/ext2.mk:46: recipe for target '/home/ymorin/dev/buildroot/O/images/rootfs.ext2' failed
    make[1]: *** [/home/ymorin/dev/buildroot/O/images/rootfs.ext2] Error 1
    Makefile:23: recipe for target '_all' failed
    make: *** [_all] Error 2

Note that only the first three lines are messages from mkfs.ext4. It also
fails for ext2 and ext3, btw, and possibly some other filesystems.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

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

* [Buildroot] [PATCH 2/3] fs: be oblivious of pre-existing xattrs
  2018-10-27  7:45 [Buildroot] [PATCH 0/3] fs: fix and better handle capabilities Yann E. MORIN
  2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
@ 2018-10-27  7:45 ` Yann E. MORIN
  2018-11-02 20:31   ` Matthew Weber
  2018-10-27  7:46 ` [Buildroot] [PATCH 3/3] fs: fix condition to create static devices Yann E. MORIN
  2 siblings, 1 reply; 25+ messages in thread
From: Yann E. MORIN @ 2018-10-27  7:45 UTC (permalink / raw)
  To: buildroot

Now that we handle permissions in each filesystems, and no longer in the
intermediate tarball, we no longer need tar to store extended attributes.

What's more, we really want it to store no attributes at all, to be sure
we don't have rogue extended attributes leaking in...

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Matthew Weber <matthew.weber@rockwellcollins.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 fs/common.mk | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/common.mk b/fs/common.mk
index 569e5d60c5..fd1e80ab93 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -45,6 +45,7 @@ ROOTFS_COMMON_TAR = $(FS_DIR)/rootfs.common.tar
 # Command to create the common tarball from the base target directory.
 define ROOTFS_COMMON_TAR_CMD
 	tar cf $(ROOTFS_COMMON_TAR) --numeric-owner \
+		--no-xattrs \
 		--exclude=$(notdir $(TARGET_DIR_WARNING_FILE)) \
 		-C $(TARGET_DIR) .
 endef
@@ -52,7 +53,9 @@ endef
 # Command to extract the common tarball into the per-rootfs target directory
 define ROOTFS_COMMON_UNTAR_CMD
 	mkdir -p $(TARGET_DIR)
-	tar xf $(ROOTFS_COMMON_TAR) -C $(TARGET_DIR)
+	tar xf $(ROOTFS_COMMON_TAR) \
+		--no-xattrs \
+		-C $(TARGET_DIR)
 endef
 
 .PHONY: rootfs-common
-- 
2.14.1

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

* [Buildroot] [PATCH 2/3] fs: be oblivious of pre-existing xattrs
  2018-10-27  7:45 ` [Buildroot] [PATCH 2/3] fs: be oblivious of pre-existing xattrs Yann E. MORIN
@ 2018-11-02 20:31   ` Matthew Weber
  0 siblings, 0 replies; 25+ messages in thread
From: Matthew Weber @ 2018-11-02 20:31 UTC (permalink / raw)
  To: buildroot

Yann,

On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Now that we handle permissions in each filesystems, and no longer in the
> intermediate tarball, we no longer need tar to store extended attributes.
>
> What's more, we really want it to store no attributes at all, to be sure
> we don't have rogue extended attributes leaking in...
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Matthew Weber <matthew.weber@rockwellcollins.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>


Tested-by: Matthew Weber <matthew.weber@rockwellcollins.com>

Ran some scenaios as a regression on existing target builds before and
after the change.  I did not use a SElinux enabled system.  I'll keep
that in mind as a scenario that probably needs checked.

> ---
>  fs/common.mk | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/common.mk b/fs/common.mk
> index 569e5d60c5..fd1e80ab93 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -45,6 +45,7 @@ ROOTFS_COMMON_TAR = $(FS_DIR)/rootfs.common.tar
>  # Command to create the common tarball from the base target directory.
>  define ROOTFS_COMMON_TAR_CMD
>         tar cf $(ROOTFS_COMMON_TAR) --numeric-owner \
> +               --no-xattrs \
>                 --exclude=$(notdir $(TARGET_DIR_WARNING_FILE)) \
>                 -C $(TARGET_DIR) .
>  endef
> @@ -52,7 +53,9 @@ endef
>  # Command to extract the common tarball into the per-rootfs target directory
>  define ROOTFS_COMMON_UNTAR_CMD
>         mkdir -p $(TARGET_DIR)
> -       tar xf $(ROOTFS_COMMON_TAR) -C $(TARGET_DIR)
> +       tar xf $(ROOTFS_COMMON_TAR) \
> +               --no-xattrs \
> +               -C $(TARGET_DIR)
>  endef
>
>  .PHONY: rootfs-common
> --
> 2.14.1
>

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

* [Buildroot] [PATCH 3/3] fs: fix condition to create static devices
  2018-10-27  7:45 [Buildroot] [PATCH 0/3] fs: fix and better handle capabilities Yann E. MORIN
  2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
  2018-10-27  7:45 ` [Buildroot] [PATCH 2/3] fs: be oblivious of pre-existing xattrs Yann E. MORIN
@ 2018-10-27  7:46 ` Yann E. MORIN
  2018-11-02 20:34   ` Matthew Weber
  2 siblings, 1 reply; 25+ messages in thread
From: Yann E. MORIN @ 2018-10-27  7:46 UTC (permalink / raw)
  To: buildroot

Currently, we parse the static devices tables even when we're not using
static device nodes; this is a left over from when we mixed devices
tables and permissions tables together. Creating package-defined device
nodes is guarded by the condition that there is at elast one devices
table.

This means that, when using static device nodes, but no device table, we
would miss on the package-defined device nodes.

We fix that by inverting the condition: static device tables are now
parsed only when using static devices, but the package-defined device
nodes are no longer conditioned to there being at least one devices
table.

Note: when there is no device table, the package-defined device ndoes
are appended to the file. That file can't exist from a previous build,
because the whole directory is rmoved at the beginning of the rule.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 fs/common.mk | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/common.mk b/fs/common.mk
index fd1e80ab93..a0699b035d 100644
--- a/fs/common.mk
+++ b/fs/common.mk
@@ -84,11 +84,11 @@ ifneq ($(ROOTFS_USERS_TABLES),)
 	cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
 endif
 	PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
+ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
 ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
 	cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
-ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
+endif
 	$(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
-endif
 	echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
 endif
 	$(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
-- 
2.14.1

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

* [Buildroot] [PATCH 3/3] fs: fix condition to create static devices
  2018-10-27  7:46 ` [Buildroot] [PATCH 3/3] fs: fix condition to create static devices Yann E. MORIN
@ 2018-11-02 20:34   ` Matthew Weber
  0 siblings, 0 replies; 25+ messages in thread
From: Matthew Weber @ 2018-11-02 20:34 UTC (permalink / raw)
  To: buildroot

Yann,

On Sat, Oct 27, 2018 at 2:46 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Currently, we parse the static devices tables even when we're not using
> static device nodes; this is a left over from when we mixed devices
> tables and permissions tables together. Creating package-defined device
> nodes is guarded by the condition that there is at elast one devices

elast -> least

> table.
>
> This means that, when using static device nodes, but no device table, we
> would miss on the package-defined device nodes.
>
> We fix that by inverting the condition: static device tables are now
> parsed only when using static devices, but the package-defined device
> nodes are no longer conditioned to there being at least one devices
> table.
>
> Note: when there is no device table, the package-defined device ndoes

ndoes -> nodes

> are appended to the file. That file can't exist from a previous build,
> because the whole directory is rmoved at the beginning of the rule.

rmoved -> removed

>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Reviewed-by: Matthew Weber <matthew.weber@rockwellcollins.com>

> ---
>  fs/common.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/common.mk b/fs/common.mk
> index fd1e80ab93..a0699b035d 100644
> --- a/fs/common.mk
> +++ b/fs/common.mk
> @@ -84,11 +84,11 @@ ifneq ($(ROOTFS_USERS_TABLES),)
>         cat $(ROOTFS_USERS_TABLES) >> $(USERS_TABLE)
>  endif
>         PATH=$(BR_PATH) $(TOPDIR)/support/scripts/mkusers $(USERS_TABLE) $(TARGET_DIR) >> $(FAKEROOT_SCRIPT)
> +ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
>  ifneq ($(ROOTFS_STATIC_DEVICE_TABLES),)
>         cat $(ROOTFS_STATIC_DEVICE_TABLES) > $(FULL_DEVICE_TABLE)
> -ifeq ($(BR2_ROOTFS_DEVICE_CREATION_STATIC),y)
> +endif
>         $(call PRINTF,$(PACKAGES_DEVICES_TABLE)) >> $(FULL_DEVICE_TABLE)
> -endif
>         echo "$(HOST_DIR)/bin/makedevs -d $(FULL_DEVICE_TABLE) $(TARGET_DIR)" >> $(FAKEROOT_SCRIPT)
>  endif
>         $(foreach s,$(call qstrip,$(BR2_ROOTFS_POST_FAKEROOT_SCRIPT)),\
> --

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

end of thread, other threads:[~2018-11-12  8:17 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-27  7:45 [Buildroot] [PATCH 0/3] fs: fix and better handle capabilities Yann E. MORIN
2018-10-27  7:45 ` [Buildroot] [PATCH 1/3] fs: apply permissions late Yann E. MORIN
2018-10-27 13:14   ` Matthew Weber
2018-10-30 20:23     ` Yann E. MORIN
2018-10-31  1:18       ` Matthew Weber
2018-10-31 21:49         ` Yann E. MORIN
2018-11-02 20:29           ` Matthew Weber
2018-11-03 13:38   ` Thomas Petazzoni
2018-11-10 17:17     ` Yann E. MORIN
2018-11-07 23:43   ` Arnout Vandecappelle
2018-11-09 20:13     ` Arnout Vandecappelle
2018-11-10 17:08       ` Yann E. MORIN
2018-11-10 17:57     ` Yann E. MORIN
2018-11-11 16:02       ` Arnout Vandecappelle
2018-11-11 16:44         ` Yann E. MORIN
2018-11-11 20:03         ` Peter Korsgaard
2018-11-11 20:02       ` Peter Korsgaard
2018-11-12  8:17         ` Yann E. MORIN
2018-11-08 22:58   ` Peter Korsgaard
2018-11-09  8:55     ` Peter Korsgaard
2018-11-10 17:07     ` Yann E. MORIN
2018-10-27  7:45 ` [Buildroot] [PATCH 2/3] fs: be oblivious of pre-existing xattrs Yann E. MORIN
2018-11-02 20:31   ` Matthew Weber
2018-10-27  7:46 ` [Buildroot] [PATCH 3/3] fs: fix condition to create static devices Yann E. MORIN
2018-11-02 20:34   ` Matthew Weber

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.