2018-02-16 20:40:18

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 00/15] extend initramfs archive format to support xattrs

Many of the Linux security/integrity features are dependent on file
metadata, stored as extended attributes (xattrs), for making decisions.
These features need to be initialized during initcall and enabled as
early as possible for complete security coverage.

Initramfs (tmpfs) supports xattrs, but newc CPIO archive format does not
support including them into the archive.

There are several ways to include xattrs for initramfs:

- Add TAR support. Complicated format and big headers looks like too
much overhead.

- Include a file manifest containing the xattrs in the CPIO. Should be
easy for initramfs because we can set xattrs at the end when all files
are extracted, but extracting such archive in userspace will be more
complicated. For example it may be necessary to set SELinux labels for
directories before extracting files into them, so manifest has to be
extracted first and then searched during each file extraction.

- Extend CPIO header to support xattrs. This seem to be the most
straight forward way. It also allows to do other useful changes
to CPIO format at the same time. E.g. increase filesize field to
support files >4GB.

This patch set extends the existing newc CPIO archive format to include
xattrs in the initramfs.

The series is based on v4.16-rc1. cpio_xattr branch is available here:
https://github.com/kontar/linux/commits/cpio_xattr

=== Patch summary ===

Documentation:
[PATCH 01/15] Documentation: add newcx initramfs format description

Refactoring to simplify adding the new format:
[PATCH 02/15] initramfs: replace states with function pointers
[PATCH 03/15] initramfs: store file name in name_buf
[PATCH 04/15] initramfs: remove unnecessary symlinks processing shortcut
[PATCH 05/15] initramfs: move files creation into separate state
[PATCH 06/15] initramfs: separate reading cpio method from header
[PATCH 07/15] initramfs: split header layout information from parsing
function

Parse newxc format:
[PATCH 08/15] initramfs: add newcx format
[PATCH 09/15] initramfs: set extended attributes

Generate newcx cpio archive:
[PATCH 10/15] gen_init_cpio: move header formatting into function
[PATCH 11/15] gen_init_cpio: add newcx format
[PATCH 12/15] gen_init_cpio: set extended attributes for newcx
[PATCH 13/15] gen_initramfs_list.sh: add -x option to enable newcx

SELinux patches used for testing. They will be sent to SELinux
maintainers separately.
[PATCH 14/15] selinux: allow setxattr on rootfs so initramfs code can
set them
[PATCH 15/15] selinux: delay sid population for rootfs till init is
complete

=== Testing ===

gen_initramfs_list.sh can be used to generate newcx CPIO archive: if
CONFIG_INITRAMFS_NEWCX is enabled CONFIG_INITRAMFS_SOURCE will be packed
into newcx archive. It is enough for basic testing, but it is not
convenient for more complex setup.

Victor have prepared a test setup with SELinux-labeled initramfs based
on Poky(Yocto) with meta-selinux layer.

Repo manifest and build instructions:
https://github.com/victorkamensky/initramfs-xattrs-manifest

Reference cpio utility patch to support newcx format could be
found as part of poky/meta-selinux testing environment at

https://raw.githubusercontent.com/victorkamensky/initramfs-xattrs-poky/rocko/meta/recipes-extended/cpio/cpio-2.12/cpio-xattrs.patch

=== History ===

The patch set is based on Mimi's series from Jan 2015:
https://www.mail-archive.com/[email protected]/msg03971.html
Latest discussion I was able to find is from Dec 2015:
https://www.mail-archive.com/[email protected]/msg04198.html

Format changes:
- increased size of filesize to 64 bits to support files >4GB.
- increased mtime field size to have 64 bits of seconds and added a
field for nanoseconds
- checksum field is replaced by xattrs_size field.

Other fields are left unchanged. See patch format description in the
patch #1.

v3 changes:
- added separate mtime nanosecond field

v2 changes:
- added documentation
- made format more consistent. In previous version a sequence of fields
in newcx header was different for symlinks and regular files (for
symlinks data field was before xattrs). It was caused by a flow
shortcut during symlink entry parsing.
- removed unused checksum field in newcx header
- removed redundant xattrcount at the beginning of xattr section
(xattrs_size is enough to determine the end of section).
- size of xattr entry in xattr section includes both name and value.
This makes format more consistent and allows to jump over an entry
without scanning for the end of name string first.
- streamlined the state machine to address the previous issue and make
it easier to add the new format
- made header parsing data-driven to remove magic numbers and make it
easier to add the new format
- eliminated unnecessary buffer allocation for every file name
- pass xattrs to gen_init_cpio via cpio_list file instead of reading
them from files during packaging. This allows to set xattrs in CPIO
even if they can't be set on a build machine.
- incorporated several bug fixes from Victor Kamensky for v1 series

Mimi Zohar (3):
initramfs: separate reading cpio method from header
initramfs: set extended attributes
gen_initramfs_list.sh: add -x option to enable newcx format

Taras Kondratiuk (10):
Documentation: add newcx initramfs format description
initramfs: replace states with function pointers
initramfs: store file name in name_buf
initramfs: remove unnecessary symlinks processing shortcut
initramfs: move files creation into separate state
initramfs: split header layout information from parsing function
initramfs: add newcx format
gen_init_cpio: move header formatting into function
gen_init_cpio: add newcx format
gen_init_cpio: set extended attributes for newcx format

Victor Kamensky (2):
selinux: allow setxattr on rootfs so initramfs code can set them
selinux: delay sid population for rootfs till init is complete

Documentation/early-userspace/buffer-format.txt | 46 ++-
init/initramfs.c | 415 +++++++++++++++++-------
scripts/gen_initramfs_list.sh | 13 +-
security/selinux/hooks.c | 19 ++
security/selinux/include/security.h | 1 +
usr/Kconfig | 11 +
usr/Makefile | 3 +-
usr/gen_init_cpio.c | 365 ++++++++++++++-------
8 files changed, 632 insertions(+), 241 deletions(-)

--
2.10.3.dirty



2018-02-16 20:36:24

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 12/14] gen_initramfs_list.sh: add -x option to enable newcx format

From: Mimi Zohar <[email protected]>

-x option populates extended attributes in cpio_list file passed to
get_init_cpio and selects newcx CPIO format.

Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: Taras Kondratiuk <[email protected]>
---
scripts/gen_initramfs_list.sh | 13 ++++++++++++-
usr/Kconfig | 11 +++++++++++
usr/Makefile | 3 ++-
3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/scripts/gen_initramfs_list.sh b/scripts/gen_initramfs_list.sh
index 86a3c0e5cfbc..cddb82f093d9 100755
--- a/scripts/gen_initramfs_list.sh
+++ b/scripts/gen_initramfs_list.sh
@@ -24,6 +24,7 @@ $0 [-o <file>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} ...
-g <gid> Group ID to map to group ID 0 (root).
<gid> is only meaningful if <cpio_source> is a
directory. "squash" forces all files to gid 0.
+ -x include file extended attributes in cpio archive.
<cpio_source> File list or directory for cpio archive.
If <cpio_source> is a .cpio file it will be used
as direct input to initramfs.
@@ -146,6 +147,9 @@ parse() {
;;
esac

+ $include_xattrs && \
+ getfattr -h -d -m - -e hex --absolute-names ${location} | \
+ sed -e '/^#/d' -e '/^$/d' -e 's/^/xattr /' >> ${output}
echo "${str}" >> ${output}

return 0
@@ -226,6 +230,8 @@ root_gid=0
dep_list=
cpio_file=
cpio_list=
+cpio_opts=
+include_xattrs=false
output="/dev/stdout"
output_file=""
is_cpio_compressed=
@@ -283,6 +289,10 @@ while [ $# -gt 0 ]; do
default_list="$arg"
${dep_list}default_initramfs
;;
+ "-x") # include extended attributers
+ cpio_opts="-x"
+ include_xattrs=true
+ ;;
"-h")
usage
exit 0
@@ -312,7 +322,8 @@ if [ ! -z ${output_file} ]; then
fi
fi
cpio_tfile="$(mktemp ${TMPDIR:-/tmp}/cpiofile.XXXXXX)"
- usr/gen_init_cpio $timestamp ${cpio_list} > ${cpio_tfile}
+ usr/gen_init_cpio $timestamp ${cpio_opts} ${cpio_list} \
+ > ${cpio_tfile}
else
cpio_tfile=${cpio_file}
fi
diff --git a/usr/Kconfig b/usr/Kconfig
index 43658b8a975e..0cc03bc4614c 100644
--- a/usr/Kconfig
+++ b/usr/Kconfig
@@ -52,6 +52,17 @@ config INITRAMFS_ROOT_GID

If you are not sure, leave it set to "0".

+config INITRAMFS_NEWCX
+ bool "Use newcx CPIO format for initramfs"
+ depends on INITRAMFS_SOURCE!=""
+ default n
+ help
+ If selected "usr/gen_init_cpio" will generate newcx CPIO archive
+ format that supports extended attributes.
+
+ See <file:Documentation/early-userspace/buffer-format.txt> for
+ more details.
+
config RD_GZIP
bool "Support initial ramdisk/ramfs compressed using gzip"
depends on BLK_DEV_INITRD
diff --git a/usr/Makefile b/usr/Makefile
index 237a028693ce..1106bfd61475 100644
--- a/usr/Makefile
+++ b/usr/Makefile
@@ -29,7 +29,8 @@ ramfs-input := $(if $(filter-out "",$(CONFIG_INITRAMFS_SOURCE)), \
$(shell echo $(CONFIG_INITRAMFS_SOURCE)),-d)
ramfs-args := \
$(if $(CONFIG_INITRAMFS_ROOT_UID), -u $(CONFIG_INITRAMFS_ROOT_UID)) \
- $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID))
+ $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID)) \
+ $(if $(CONFIG_INITRAMFS_NEWCX), -x)

# $(datafile_d_y) is used to identify all files included
# in initramfs and to detect if any files are added/removed.
--
2.10.3.dirty


2018-02-16 20:36:26

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 13/14] selinux: allow setxattr on rootfs so initramfs code can set them

From: Victor Kamensky <[email protected]>

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code.

Solution enable SBLABEL_MNT on rootfs even if secrurity server is
not initialized yet.

Signed-off-by: Victor Kamensky <[email protected]>
---
security/selinux/hooks.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..f3fe65589f02 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -706,6 +706,18 @@ static int selinux_set_mnt_opts(struct super_block *sb,

if (!ss_initialized) {
if (!num_opts) {
+ /*
+ * Special handling for rootfs. Is genfs but supports
+ * setting SELinux context on in-core inodes.
+ *
+ * Chicken and egg problem: policy may reside in rootfs
+ * but for initramfs code to fill in attributes, it
+ * needs selinux to allow that.
+ */
+ if (!strncmp(sb->s_type->name, "rootfs",
+ sizeof("rootfs")))
+ sbsec->flags |= SBLABEL_MNT;
+
/* Defer initialization until selinux_complete_init,
after the initial policy is loaded and the security
server is ready to handle calls. */
--
2.10.3.dirty


2018-02-16 20:36:52

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete

From: Victor Kamensky <[email protected]>

With initramfs cpio format that supports extended attributes
we need to skip sid population on sys_lsetxattr call from
initramfs for rootfs if security server is not initialized yet.

Otherwise callback in selinux_inode_post_setxattr will try to
translate give security.selinux label into sid context and since
security server is not available yet inode will receive default
sid (typically kernel_t). Note that in the same time proper
label will be stored in inode xattrs. Later, since inode sid
would be already populated system will never look back at
actual xattrs. But if we skip sid population for rootfs and
we have policy that direct use of xattrs for rootfs, proper
sid will be filled in from extended attributes one node is
accessed and server is initialized.

Note new DELAYAFTERINIT_MNT super block flag is introduced
to only mark rootfs for such behavior. For other types of
tmpfs original logic is still used.

Signed-off-by: Victor Kamensky <[email protected]>
---
security/selinux/hooks.c | 9 ++++++++-
security/selinux/include/security.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f3fe65589f02..bb25268f734e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -716,7 +716,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
*/
if (!strncmp(sb->s_type->name, "rootfs",
sizeof("rootfs")))
- sbsec->flags |= SBLABEL_MNT;
+ sbsec->flags |= SBLABEL_MNT|DELAYAFTERINIT_MNT;

/* Defer initialization until selinux_complete_init,
after the initial policy is loaded and the security
@@ -3253,6 +3253,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
{
struct inode *inode = d_backing_inode(dentry);
struct inode_security_struct *isec;
+ struct superblock_security_struct *sbsec;
u32 newsid;
int rc;

@@ -3261,6 +3262,12 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
return;
}

+ if (!ss_initialized) {
+ sbsec = inode->i_sb->s_security;
+ if (sbsec->flags & DELAYAFTERINIT_MNT)
+ return;
+ }
+
rc = security_context_to_sid_force(value, size, &newsid);
if (rc) {
printk(KERN_ERR "SELinux: unable to map context to SID"
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 02f0412d42f2..585acfd6cbcf 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -52,6 +52,7 @@
#define ROOTCONTEXT_MNT 0x04
#define DEFCONTEXT_MNT 0x08
#define SBLABEL_MNT 0x10
+#define DELAYAFTERINIT_MNT 0x20
/* Non-mount related flags */
#define SE_SBINITIALIZED 0x0100
#define SE_SBPROC 0x0200
--
2.10.3.dirty


2018-02-16 20:36:54

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 13/15] gen_initramfs_list.sh: add -x option to enable newcx format

From: Mimi Zohar <[email protected]>

-x option populates extended attributes in cpio_list file passed to
get_init_cpio and selects newcx CPIO format.

Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: Taras Kondratiuk <[email protected]>
---
scripts/gen_initramfs_list.sh | 13 ++++++++++++-
usr/Kconfig | 11 +++++++++++
usr/Makefile | 3 ++-
3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/scripts/gen_initramfs_list.sh b/scripts/gen_initramfs_list.sh
index 86a3c0e5cfbc..cddb82f093d9 100755
--- a/scripts/gen_initramfs_list.sh
+++ b/scripts/gen_initramfs_list.sh
@@ -24,6 +24,7 @@ $0 [-o <file>] [-u <uid>] [-g <gid>] {-d | <cpio_source>} ...
-g <gid> Group ID to map to group ID 0 (root).
<gid> is only meaningful if <cpio_source> is a
directory. "squash" forces all files to gid 0.
+ -x include file extended attributes in cpio archive.
<cpio_source> File list or directory for cpio archive.
If <cpio_source> is a .cpio file it will be used
as direct input to initramfs.
@@ -146,6 +147,9 @@ parse() {
;;
esac

+ $include_xattrs && \
+ getfattr -h -d -m - -e hex --absolute-names ${location} | \
+ sed -e '/^#/d' -e '/^$/d' -e 's/^/xattr /' >> ${output}
echo "${str}" >> ${output}

return 0
@@ -226,6 +230,8 @@ root_gid=0
dep_list=
cpio_file=
cpio_list=
+cpio_opts=
+include_xattrs=false
output="/dev/stdout"
output_file=""
is_cpio_compressed=
@@ -283,6 +289,10 @@ while [ $# -gt 0 ]; do
default_list="$arg"
${dep_list}default_initramfs
;;
+ "-x") # include extended attributers
+ cpio_opts="-x"
+ include_xattrs=true
+ ;;
"-h")
usage
exit 0
@@ -312,7 +322,8 @@ if [ ! -z ${output_file} ]; then
fi
fi
cpio_tfile="$(mktemp ${TMPDIR:-/tmp}/cpiofile.XXXXXX)"
- usr/gen_init_cpio $timestamp ${cpio_list} > ${cpio_tfile}
+ usr/gen_init_cpio $timestamp ${cpio_opts} ${cpio_list} \
+ > ${cpio_tfile}
else
cpio_tfile=${cpio_file}
fi
diff --git a/usr/Kconfig b/usr/Kconfig
index 43658b8a975e..0cc03bc4614c 100644
--- a/usr/Kconfig
+++ b/usr/Kconfig
@@ -52,6 +52,17 @@ config INITRAMFS_ROOT_GID

If you are not sure, leave it set to "0".

+config INITRAMFS_NEWCX
+ bool "Use newcx CPIO format for initramfs"
+ depends on INITRAMFS_SOURCE!=""
+ default n
+ help
+ If selected "usr/gen_init_cpio" will generate newcx CPIO archive
+ format that supports extended attributes.
+
+ See <file:Documentation/early-userspace/buffer-format.txt> for
+ more details.
+
config RD_GZIP
bool "Support initial ramdisk/ramfs compressed using gzip"
depends on BLK_DEV_INITRD
diff --git a/usr/Makefile b/usr/Makefile
index 237a028693ce..1106bfd61475 100644
--- a/usr/Makefile
+++ b/usr/Makefile
@@ -29,7 +29,8 @@ ramfs-input := $(if $(filter-out "",$(CONFIG_INITRAMFS_SOURCE)), \
$(shell echo $(CONFIG_INITRAMFS_SOURCE)),-d)
ramfs-args := \
$(if $(CONFIG_INITRAMFS_ROOT_UID), -u $(CONFIG_INITRAMFS_ROOT_UID)) \
- $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID))
+ $(if $(CONFIG_INITRAMFS_ROOT_GID), -g $(CONFIG_INITRAMFS_ROOT_GID)) \
+ $(if $(CONFIG_INITRAMFS_NEWCX), -x)

# $(datafile_d_y) is used to identify all files included
# in initramfs and to detect if any files are added/removed.
--
2.10.3.dirty


2018-02-16 20:37:28

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 14/15] selinux: allow setxattr on rootfs so initramfs code can set them

From: Victor Kamensky <[email protected]>

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code.

Solution enable SBLABEL_MNT on rootfs even if secrurity server is
not initialized yet.

Signed-off-by: Victor Kamensky <[email protected]>
---
security/selinux/hooks.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..f3fe65589f02 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -706,6 +706,18 @@ static int selinux_set_mnt_opts(struct super_block *sb,

if (!ss_initialized) {
if (!num_opts) {
+ /*
+ * Special handling for rootfs. Is genfs but supports
+ * setting SELinux context on in-core inodes.
+ *
+ * Chicken and egg problem: policy may reside in rootfs
+ * but for initramfs code to fill in attributes, it
+ * needs selinux to allow that.
+ */
+ if (!strncmp(sb->s_type->name, "rootfs",
+ sizeof("rootfs")))
+ sbsec->flags |= SBLABEL_MNT;
+
/* Defer initialization until selinux_complete_init,
after the initial policy is loaded and the security
server is ready to handle calls. */
--
2.10.3.dirty


2018-02-16 20:37:35

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 14/14] selinux: delay sid population for rootfs till init is complete

From: Victor Kamensky <[email protected]>

With initramfs cpio format that supports extended attributes
we need to skip sid population on sys_lsetxattr call from
initramfs for rootfs if security server is not initialized yet.

Otherwise callback in selinux_inode_post_setxattr will try to
translate give security.selinux label into sid context and since
security server is not available yet inode will receive default
sid (typically kernel_t). Note that in the same time proper
label will be stored in inode xattrs. Later, since inode sid
would be already populated system will never look back at
actual xattrs. But if we skip sid population for rootfs and
we have policy that direct use of xattrs for rootfs, proper
sid will be filled in from extended attributes one node is
accessed and server is initialized.

Note new DELAYAFTERINIT_MNT super block flag is introduced
to only mark rootfs for such behavior. For other types of
tmpfs original logic is still used.

Signed-off-by: Victor Kamensky <[email protected]>
---
security/selinux/hooks.c | 9 ++++++++-
security/selinux/include/security.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f3fe65589f02..bb25268f734e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -716,7 +716,7 @@ static int selinux_set_mnt_opts(struct super_block *sb,
*/
if (!strncmp(sb->s_type->name, "rootfs",
sizeof("rootfs")))
- sbsec->flags |= SBLABEL_MNT;
+ sbsec->flags |= SBLABEL_MNT|DELAYAFTERINIT_MNT;

/* Defer initialization until selinux_complete_init,
after the initial policy is loaded and the security
@@ -3253,6 +3253,7 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
{
struct inode *inode = d_backing_inode(dentry);
struct inode_security_struct *isec;
+ struct superblock_security_struct *sbsec;
u32 newsid;
int rc;

@@ -3261,6 +3262,12 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
return;
}

+ if (!ss_initialized) {
+ sbsec = inode->i_sb->s_security;
+ if (sbsec->flags & DELAYAFTERINIT_MNT)
+ return;
+ }
+
rc = security_context_to_sid_force(value, size, &newsid);
if (rc) {
printk(KERN_ERR "SELinux: unable to map context to SID"
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 02f0412d42f2..585acfd6cbcf 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -52,6 +52,7 @@
#define ROOTCONTEXT_MNT 0x04
#define DEFCONTEXT_MNT 0x08
#define SBLABEL_MNT 0x10
+#define DELAYAFTERINIT_MNT 0x20
/* Non-mount related flags */
#define SE_SBINITIALIZED 0x0100
#define SE_SBPROC 0x0200
--
2.10.3.dirty


2018-02-16 20:37:41

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 08/15] initramfs: add newcx format

Add 'newcx' format that adds extended attributes and increased size of
c_mtime and c_filesize fields.

Refer to Documentation/early-userspace/buffer-format.txt for detailed
format description.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 121 +++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 96 insertions(+), 25 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 7f0bbfde94e3..0407e199352e 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -54,6 +54,7 @@ static void __init error(char *x)
/* link hash */

#define N_ALIGN(len) ((((len) + 1) & ~3) + 2)
+#define X_ALIGN(len) ((len + 3) & ~3)

static __initdata struct hash {
int ino, minor, major;
@@ -109,14 +110,11 @@ static void __init free_hash(void)
}
}

-static long __init do_utime(char *filename, time64_t mtime)
+static long __init do_utime(char *filename, struct timespec64 *mtime)
{
struct timespec64 t[2];

- t[0].tv_sec = mtime;
- t[0].tv_nsec = 0;
- t[1].tv_sec = mtime;
- t[1].tv_nsec = 0;
+ t[0] = t[1] = *mtime;

return do_utimes(AT_FDCWD, filename, t, AT_SYMLINK_NOFOLLOW);
}
@@ -125,17 +123,17 @@ static __initdata LIST_HEAD(dir_list);
struct dir_entry {
struct list_head list;
char *name;
- time64_t mtime;
+ struct timespec64 mtime;
};

-static void __init dir_add(const char *name, time64_t mtime)
+static void __init dir_add(const char *name, struct timespec64 *mtime)
{
struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
if (!de)
panic("can't allocate dir_entry buffer");
INIT_LIST_HEAD(&de->list);
de->name = kstrdup(name, GFP_KERNEL);
- de->mtime = mtime;
+ de->mtime = *mtime;
list_add(&de->list, &dir_list);
}

@@ -144,17 +142,18 @@ static void __init dir_utime(void)
struct dir_entry *de, *tmp;
list_for_each_entry_safe(de, tmp, &dir_list, list) {
list_del(&de->list);
- do_utime(de->name, de->mtime);
+ do_utime(de->name, &de->mtime);
kfree(de->name);
kfree(de);
}
}

/* cpio header parsing */
-static __initdata time64_t mtime;
+static __initdata struct timespec64 mtime;
static __initdata u32 ino, major, minor, nlink, rmajor, rminor;
static __initdata umode_t mode;
-static __initdata u32 body_len, name_len;
+static __initdata u32 name_len, xattr_len;
+static __initdata u64 body_len;
static __initdata uid_t uid;
static __initdata gid_t gid;
static __initdata u32 mode_u32;
@@ -167,6 +166,12 @@ struct cpio_hdr_field {
const char *name;
};

+static __initdata enum cpio_format {
+ CPIO_NO_MAGIC,
+ CPIO_NEWC,
+ CPIO_NEWCX,
+} cpio_format;
+
#define HDR_FIELD(type, field, variable) \
{ .offset = offsetof(type, field) + \
BUILD_BUG_ON_ZERO(sizeof(*(variable))*2 < FIELD_SIZEOF(type, field)),\
@@ -177,9 +182,11 @@ struct cpio_hdr_field {

#define NEWC_FIELD(field, variable) \
HDR_FIELD(struct cpio_newc_header, field, variable)
+#define NEWCX_FIELD(field, variable) \
+ HDR_FIELD(struct cpio_newcx_header, field, variable)

-#define CPIO_MAX_HEADER_SIZE sizeof(struct cpio_newc_header)
-#define CPIO_MAX_FIELD_SIZE 8
+#define CPIO_MAX_HEADER_SIZE sizeof(struct cpio_newcx_header)
+#define CPIO_MAX_FIELD_SIZE 16
#define CPIO_MAGIC_SIZE 6

struct cpio_newc_header {
@@ -204,7 +211,7 @@ static struct cpio_hdr_field cpio_newc_header_info[] __initdata = {
NEWC_FIELD(c_uid, &uid),
NEWC_FIELD(c_gid, &gid),
NEWC_FIELD(c_nlink, &nlink),
- NEWC_FIELD(c_mtime, &mtime),
+ NEWC_FIELD(c_mtime, &mtime.tv_sec),
NEWC_FIELD(c_filesize, &body_len),
NEWC_FIELD(c_devmajor, &major),
NEWC_FIELD(c_devminor, &minor),
@@ -214,10 +221,46 @@ static struct cpio_hdr_field cpio_newc_header_info[] __initdata = {
{ 0 },
};

+struct cpio_newcx_header {
+ char c_ino[8];
+ char c_mode[8];
+ char c_uid[8];
+ char c_gid[8];
+ char c_nlink[8];
+ char c_mtime[16];
+ char c_mtime_nsec[8];
+ char c_filesize[16];
+ char c_devmajor[8];
+ char c_devminor[8];
+ char c_rdevmajor[8];
+ char c_rdevminor[8];
+ char c_namesize[8];
+ char c_xattrsize[8];
+};
+
+static struct cpio_hdr_field cpio_newcx_header_info[] __initdata = {
+ NEWCX_FIELD(c_ino, &ino),
+ NEWCX_FIELD(c_mode, &mode_u32),
+ NEWCX_FIELD(c_uid, &uid),
+ NEWCX_FIELD(c_gid, &gid),
+ NEWCX_FIELD(c_nlink, &nlink),
+ NEWCX_FIELD(c_mtime, &mtime.tv_sec),
+ NEWCX_FIELD(c_mtime_nsec, &mtime.tv_nsec),
+ NEWCX_FIELD(c_filesize, &body_len),
+ NEWCX_FIELD(c_devmajor, &major),
+ NEWCX_FIELD(c_devminor, &minor),
+ NEWCX_FIELD(c_rdevmajor, &rmajor),
+ NEWCX_FIELD(c_rdevminor, &rminor),
+ NEWCX_FIELD(c_namesize, &name_len),
+ NEWCX_FIELD(c_xattrsize, &xattr_len),
+ { 0 },
+};
+
static void __init parse_header(char *s)
{
char buf[CPIO_MAX_FIELD_SIZE + 1];
- struct cpio_hdr_field *field = cpio_newc_header_info;
+ struct cpio_hdr_field *field = (cpio_format == CPIO_NEWC) ?
+ cpio_newc_header_info : cpio_newcx_header_info;

while (field->size) {
int ret = 0;
@@ -243,7 +286,12 @@ static void __init parse_header(char *s)
pr_err("invalid cpio header field (%d)", ret);
field++;
}
+
mode = mode_u32;
+ if (cpio_format != CPIO_NEWCX) {
+ xattr_len = 0;
+ mtime.tv_nsec = 0;
+ }
}

/* FSM */
@@ -254,6 +302,7 @@ static int __init do_format(void);
static int __init do_header(void);
static int __init do_skip(void);
static int __init do_name(void);
+static int __init do_xattrs(void);
static int __init do_create(void);
static int __init do_copy(void);
static int __init do_symlink(void);
@@ -291,7 +340,7 @@ static void __init read_into(char *buf, unsigned size, fsm_state_t next)
}
}

-static __initdata char *header_buf, *symlink_buf, *name_buf;
+static __initdata char *header_buf, *symlink_buf, *name_buf, *xattr_buf;

static int __init do_start(void)
{
@@ -315,22 +364,34 @@ static int __init do_collect(void)

static int __init do_format(void)
{
- if (memcmp(collected, "070707", CPIO_MAGIC_SIZE) == 0) {
+ int header_size = 0;
+
+ cpio_format = CPIO_NO_MAGIC;
+
+ if (!memcmp(collected, "070707", CPIO_MAGIC_SIZE)) {
error("incorrect cpio method used: use -H newc option");
return 1;
+ } else if (!memcmp(collected, "070701", CPIO_MAGIC_SIZE)) {
+ cpio_format = CPIO_NEWC;
+ header_size = sizeof(struct cpio_newc_header);
+ } else if (!memcmp(collected, "070703", CPIO_MAGIC_SIZE)) {
+ cpio_format = CPIO_NEWCX;
+ header_size = sizeof(struct cpio_newcx_header);
}
- if (memcmp(collected, "070701", CPIO_MAGIC_SIZE)) {
+
+ if (cpio_format == CPIO_NO_MAGIC) {
error("no cpio magic");
return 1;
}
- read_into(header_buf, sizeof(struct cpio_newc_header), do_header);
+ read_into(header_buf, header_size, do_header);
return 0;
}

static int __init do_header(void)
{
parse_header(collected);
- next_header = this_header + N_ALIGN(name_len) + body_len;
+ next_header = this_header + N_ALIGN(name_len) + X_ALIGN(xattr_len) +
+ body_len;
next_header = (next_header + 3) & ~3;
state = do_skip;
if (name_len <= 0 || name_len > PATH_MAX)
@@ -400,9 +461,17 @@ static int __init do_name(void)
}
memcpy_optional(name_buf, collected, N_ALIGN(name_len));
state = do_create;
+ if (xattr_len > 0)
+ read_into(xattr_buf, X_ALIGN(xattr_len), do_xattrs);
return 0;
}

+static int __init do_xattrs(void)
+{
+ /* Do nothing for now */
+ state = do_create;
+ return 0;
+}

static __initdata int wfd;

@@ -431,7 +500,7 @@ static int __init do_create(void)
sys_mkdir(name_buf, mode);
sys_chown(name_buf, uid, gid);
sys_chmod(name_buf, mode);
- dir_add(name_buf, mtime);
+ dir_add(name_buf, &mtime);
} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
S_ISFIFO(mode) || S_ISSOCK(mode)) {
if (maybe_link(name_buf) == 0) {
@@ -439,7 +508,7 @@ static int __init do_create(void)
sys_mknod(name_buf, mode, rdev);
sys_chown(name_buf, uid, gid);
sys_chmod(name_buf, mode);
- do_utime(name_buf, mtime);
+ do_utime(name_buf, &mtime);
}
} else if (S_ISLNK(mode)) {
if (body_len > PATH_MAX)
@@ -455,7 +524,7 @@ static int __init do_copy(void)
if (xwrite(wfd, victim, body_len) != body_len)
error("write error");
sys_close(wfd);
- do_utime(name_buf, mtime);
+ do_utime(name_buf, &mtime);
eat(body_len);
state = do_skip;
return 0;
@@ -475,7 +544,7 @@ static int __init do_symlink(void)
clean_path(name_buf, 0);
sys_symlink(symlink_buf, name_buf);
sys_lchown(name_buf, uid, gid);
- do_utime(name_buf, mtime);
+ do_utime(name_buf, &mtime);
state = do_skip;
next_state = do_reset;
return 0;
@@ -529,8 +598,9 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
header_buf = kmalloc(CPIO_MAX_HEADER_SIZE, GFP_KERNEL);
symlink_buf = kmalloc(PATH_MAX + 1, GFP_KERNEL);
name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);
+ xattr_buf = kmalloc(PAGE_SIZE, GFP_KERNEL);

- if (!header_buf || !symlink_buf || !name_buf)
+ if (!header_buf || !symlink_buf || !name_buf || !xattr_buf)
panic("can't allocate buffers");

state = do_start;
@@ -575,6 +645,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
len -= my_inptr;
}
dir_utime();
+ kfree(xattr_buf);
kfree(name_buf);
kfree(symlink_buf);
kfree(header_buf);
--
2.10.3.dirty


2018-02-16 20:38:20

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 10/15] gen_init_cpio: move header formatting into function

CPIO header is generated in multiple places with the same sprintf()
format string. Move formatting into a single function in preparation
to adding a new cpio format.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
usr/gen_init_cpio.c | 186 ++++++++++++++++++++++++++--------------------------
1 file changed, 92 insertions(+), 94 deletions(-)

diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index 03b21189d58b..7a2a6d85345d 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -64,34 +64,55 @@ static void push_rest(const char *name)
}
}

-static void push_hdr(const char *s)
+struct cpio_header {
+ unsigned int ino;
+ unsigned int mode;
+ uid_t uid;
+ gid_t gid;
+ unsigned int nlink;
+ time_t mtime;
+ size_t filesize;
+ int devmajor;
+ int devminor;
+ int rdevmajor;
+ int rdevminor;
+ size_t namesize;
+ unsigned int check;
+};
+
+static void push_hdr(const struct cpio_header *hdr)
{
+ char s[256];
+
+ sprintf(s, "%s%08X%08X%08lX%08lX%08X%08lX"
+ "%08X%08X%08X%08X%08X%08X%08X",
+ "070701",
+ hdr->ino,
+ hdr->mode,
+ (long)hdr->uid,
+ (long)hdr->gid,
+ hdr->nlink,
+ (long)hdr->mtime,
+ (unsigned int)hdr->filesize,
+ hdr->devmajor,
+ hdr->devminor,
+ hdr->rdevmajor,
+ hdr->rdevminor,
+ (unsigned int)hdr->namesize,
+ hdr->check);
fputs(s, stdout);
offset += 110;
}

static void cpio_trailer(void)
{
- char s[256];
const char name[] = "TRAILER!!!";
+ struct cpio_header hdr = {
+ .nlink = 1,
+ .namesize = strlen(name)+1,
+ };

- sprintf(s, "%s%08X%08X%08lX%08lX%08X%08lX"
- "%08X%08X%08X%08X%08X%08X%08X",
- "070701", /* magic */
- 0, /* ino */
- 0, /* mode */
- (long) 0, /* uid */
- (long) 0, /* gid */
- 1, /* nlink */
- (long) 0, /* mtime */
- 0, /* filesize */
- 0, /* major */
- 0, /* minor */
- 0, /* rmajor */
- 0, /* rminor */
- (unsigned)strlen(name)+1, /* namesize */
- 0); /* chksum */
- push_hdr(s);
+ push_hdr(&hdr);
push_rest(name);

while (offset % 512) {
@@ -103,27 +124,21 @@ static void cpio_trailer(void)
static int cpio_mkslink(const char *name, const char *target,
unsigned int mode, uid_t uid, gid_t gid)
{
- char s[256];
-
if (name[0] == '/')
name++;
- sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
- "%08X%08X%08X%08X%08X%08X%08X",
- "070701", /* magic */
- ino++, /* ino */
- S_IFLNK | mode, /* mode */
- (long) uid, /* uid */
- (long) gid, /* gid */
- 1, /* nlink */
- (long) default_mtime, /* mtime */
- (unsigned)strlen(target)+1, /* filesize */
- 3, /* major */
- 1, /* minor */
- 0, /* rmajor */
- 0, /* rminor */
- (unsigned)strlen(name) + 1,/* namesize */
- 0); /* chksum */
- push_hdr(s);
+ struct cpio_header hdr = {
+ .ino = ino++,
+ .mode = S_IFLNK | mode,
+ .uid = uid,
+ .gid = gid,
+ .nlink = 1,
+ .mtime = default_mtime,
+ .filesize = strlen(target)+1,
+ .devmajor = 3,
+ .devminor = 1,
+ .namesize = strlen(name)+1,
+ };
+ push_hdr(&hdr);
push_string(name);
push_pad();
push_string(target);
@@ -152,27 +167,20 @@ static int cpio_mkslink_line(const char *line)
static int cpio_mkgeneric(const char *name, unsigned int mode,
uid_t uid, gid_t gid)
{
- char s[256];
-
if (name[0] == '/')
name++;
- sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
- "%08X%08X%08X%08X%08X%08X%08X",
- "070701", /* magic */
- ino++, /* ino */
- mode, /* mode */
- (long) uid, /* uid */
- (long) gid, /* gid */
- 2, /* nlink */
- (long) default_mtime, /* mtime */
- 0, /* filesize */
- 3, /* major */
- 1, /* minor */
- 0, /* rmajor */
- 0, /* rminor */
- (unsigned)strlen(name) + 1,/* namesize */
- 0); /* chksum */
- push_hdr(s);
+ struct cpio_header hdr = {
+ .ino = ino++,
+ .mode = mode,
+ .uid = uid,
+ .gid = gid,
+ .nlink = 2,
+ .mtime = default_mtime,
+ .devmajor = 3,
+ .devminor = 1,
+ .namesize = strlen(name)+1,
+ };
+ push_hdr(&hdr);
push_rest(name);
return 0;
}
@@ -241,8 +249,6 @@ static int cpio_mknod(const char *name, unsigned int mode,
uid_t uid, gid_t gid, char dev_type,
unsigned int maj, unsigned int min)
{
- char s[256];
-
if (dev_type == 'b')
mode |= S_IFBLK;
else
@@ -250,23 +256,20 @@ static int cpio_mknod(const char *name, unsigned int mode,

if (name[0] == '/')
name++;
- sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
- "%08X%08X%08X%08X%08X%08X%08X",
- "070701", /* magic */
- ino++, /* ino */
- mode, /* mode */
- (long) uid, /* uid */
- (long) gid, /* gid */
- 1, /* nlink */
- (long) default_mtime, /* mtime */
- 0, /* filesize */
- 3, /* major */
- 1, /* minor */
- maj, /* rmajor */
- min, /* rminor */
- (unsigned)strlen(name) + 1,/* namesize */
- 0); /* chksum */
- push_hdr(s);
+ struct cpio_header hdr = {
+ .ino = ino++,
+ .mode = mode,
+ .uid = uid,
+ .gid = gid,
+ .nlink = 1,
+ .mtime = default_mtime,
+ .devmajor = 3,
+ .devminor = 1,
+ .rdevmajor = maj,
+ .rdevminor = min,
+ .namesize = strlen(name)+1,
+ };
+ push_hdr(&hdr);
push_rest(name);
return 0;
}
@@ -296,7 +299,6 @@ static int cpio_mkfile(const char *name, const char *location,
unsigned int mode, uid_t uid, gid_t gid,
unsigned int nlinks)
{
- char s[256];
char *filebuf = NULL;
struct stat buf;
long size;
@@ -340,23 +342,19 @@ static int cpio_mkfile(const char *name, const char *location,
if (name[0] == '/')
name++;
namesize = strlen(name) + 1;
- sprintf(s,"%s%08X%08X%08lX%08lX%08X%08lX"
- "%08lX%08X%08X%08X%08X%08X%08X",
- "070701", /* magic */
- ino, /* ino */
- mode, /* mode */
- (long) uid, /* uid */
- (long) gid, /* gid */
- nlinks, /* nlink */
- (long) buf.st_mtime, /* mtime */
- size, /* filesize */
- 3, /* major */
- 1, /* minor */
- 0, /* rmajor */
- 0, /* rminor */
- namesize, /* namesize */
- 0); /* chksum */
- push_hdr(s);
+ struct cpio_header hdr = {
+ .ino = ino,
+ .mode = mode,
+ .uid = uid,
+ .gid = gid,
+ .nlink = nlinks,
+ .mtime = buf.st_mtime,
+ .filesize = size,
+ .devmajor = 3,
+ .devminor = 1,
+ .namesize = namesize,
+ };
+ push_hdr(&hdr);
push_string(name);
push_pad();

--
2.10.3.dirty


2018-02-16 20:38:26

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 11/15] gen_init_cpio: add newcx format

Add "newcx" format that supports extended attributes and has increased
size of c_mtime and c_filesize fields.

Added -x option to select "newcx" format. Default is "newc".

Refer to Documentation/early-userspace/buffer-format.txt for detailed
format description.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
usr/gen_init_cpio.c | 69 +++++++++++++++++++++++++++++++++++++----------------
1 file changed, 48 insertions(+), 21 deletions(-)

diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index 7a2a6d85345d..25afd5b4af77 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -21,6 +21,8 @@
#define xstr(s) #s
#define str(s) xstr(s)

+static int newcx;
+static unsigned int cpio_hdr_size;
static unsigned int offset;
static unsigned int ino = 721;
static time_t default_mtime;
@@ -56,7 +58,7 @@ static void push_rest(const char *name)
putchar(0);
offset += name_len;

- tmp_ofs = name_len + 110;
+ tmp_ofs = name_len + cpio_hdr_size;
while (tmp_ofs & 3) {
putchar(0);
offset++;
@@ -77,6 +79,7 @@ struct cpio_header {
int rdevmajor;
int rdevminor;
size_t namesize;
+ size_t xattrsize;
unsigned int check;
};

@@ -84,24 +87,44 @@ static void push_hdr(const struct cpio_header *hdr)
{
char s[256];

- sprintf(s, "%s%08X%08X%08lX%08lX%08X%08lX"
- "%08X%08X%08X%08X%08X%08X%08X",
- "070701",
- hdr->ino,
- hdr->mode,
- (long)hdr->uid,
- (long)hdr->gid,
- hdr->nlink,
- (long)hdr->mtime,
- (unsigned int)hdr->filesize,
- hdr->devmajor,
- hdr->devminor,
- hdr->rdevmajor,
- hdr->rdevminor,
- (unsigned int)hdr->namesize,
- hdr->check);
+ if (newcx) {
+ sprintf(s, "%s%08X%08X%08lX%08lX%08X%016llX%08X"
+ "%016llX%08X%08X%08X%08X%08X%08X",
+ "070703",
+ hdr->ino,
+ hdr->mode,
+ (long)hdr->uid,
+ (long)hdr->gid,
+ hdr->nlink,
+ (long long)hdr->mtime,
+ 0,
+ (long long)hdr->filesize,
+ hdr->devmajor,
+ hdr->devminor,
+ hdr->rdevmajor,
+ hdr->rdevminor,
+ (unsigned int)hdr->namesize,
+ (unsigned int)hdr->xattrsize);
+ } else {
+ sprintf(s, "%s%08X%08X%08lX%08lX%08X%08lX"
+ "%08X%08X%08X%08X%08X%08X%08X",
+ "070701",
+ hdr->ino,
+ hdr->mode,
+ (long)hdr->uid,
+ (long)hdr->gid,
+ hdr->nlink,
+ (long)hdr->mtime,
+ (unsigned int)hdr->filesize,
+ hdr->devmajor,
+ hdr->devminor,
+ hdr->rdevmajor,
+ hdr->rdevminor,
+ (unsigned int)hdr->namesize,
+ hdr->check);
+ }
fputs(s, stdout);
- offset += 110;
+ offset += cpio_hdr_size;
}

static void cpio_trailer(void)
@@ -301,7 +324,7 @@ static int cpio_mkfile(const char *name, const char *location,
{
char *filebuf = NULL;
struct stat buf;
- long size;
+ size_t size;
int file = -1;
int retval;
int rc = -1;
@@ -450,7 +473,7 @@ static int cpio_mkfile_line(const char *line)
static void usage(const char *prog)
{
fprintf(stderr, "Usage:\n"
- "\t%s [-t <timestamp>] <cpio_list>\n"
+ "\t%s [-t <timestamp>] [-x] <cpio_list>\n"
"\n"
"<cpio_list> is a file containing newline separated entries that\n"
"describe the files to be included in the initramfs archive:\n"
@@ -527,7 +550,7 @@ int main (int argc, char *argv[])

default_mtime = time(NULL);
while (1) {
- int opt = getopt(argc, argv, "t:h");
+ int opt = getopt(argc, argv, "t:h:x");
char *invalid;

if (opt == -1)
@@ -542,12 +565,16 @@ int main (int argc, char *argv[])
exit(1);
}
break;
+ case 'x':
+ newcx = 1;
+ break;
case 'h':
case '?':
usage(argv[0]);
exit(opt == 'h' ? 0 : 1);
}
}
+ cpio_hdr_size = newcx ? 134 : 110;

if (argc - optind != 1) {
usage(argv[0]);
--
2.10.3.dirty


2018-02-16 20:38:46

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 09/15] initramfs: set extended attributes

From: Mimi Zohar <[email protected]>

This patch writes out the extended attributes included in the cpio file.
As the "security.ima" xattr needs to be written after the file data.
this patch separates extracting and setting the xattrs by defining new
do_setxattrs state.

[kamensky: fixed restoring of xattrs for symbolic links by using
sys_lsetxattr() instead of sys_setxattr()]

Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: Victor Kamensky <[email protected]>
Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 0407e199352e..ac636097aee5 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -306,6 +306,7 @@ static int __init do_xattrs(void);
static int __init do_create(void);
static int __init do_copy(void);
static int __init do_symlink(void);
+static int __init do_setxattrs(void);
static int __init do_reset(void);

typedef int (*fsm_state_t)(void);
@@ -468,7 +469,7 @@ static int __init do_name(void)

static int __init do_xattrs(void)
{
- /* Do nothing for now */
+ memcpy_optional(xattr_buf, collected, xattr_len);
state = do_create;
return 0;
}
@@ -477,8 +478,7 @@ static __initdata int wfd;

static int __init do_create(void)
{
- state = do_skip;
- next_state = do_reset;
+ state = do_setxattrs;
clean_path(name_buf, mode);
if (S_ISREG(mode)) {
int ml = maybe_link(name_buf);
@@ -511,8 +511,11 @@ static int __init do_create(void)
do_utime(name_buf, &mtime);
}
} else if (S_ISLNK(mode)) {
- if (body_len > PATH_MAX)
+ if (body_len > PATH_MAX) {
+ state = do_skip;
+ next_state = do_reset;
return 0;
+ }
read_into(symlink_buf, body_len, do_symlink);
}
return 0;
@@ -526,7 +529,7 @@ static int __init do_copy(void)
sys_close(wfd);
do_utime(name_buf, &mtime);
eat(body_len);
- state = do_skip;
+ state = do_setxattrs;
return 0;
} else {
if (xwrite(wfd, victim, byte_count) != byte_count)
@@ -545,8 +548,52 @@ static int __init do_symlink(void)
sys_symlink(symlink_buf, name_buf);
sys_lchown(name_buf, uid, gid);
do_utime(name_buf, &mtime);
+ state = do_setxattrs;
+ return 0;
+}
+
+struct xattr_hdr {
+ char c_size[8]; /* total size including c_size field */
+ char c_data[]; /* <name>\0<value> */
+};
+
+static int __init do_setxattrs(void)
+{
+ char *buf = xattr_buf;
+ char *bufend = buf + xattr_len;
+ struct xattr_hdr *hdr;
+ char str[sizeof(hdr->c_size) + 1];
+
state = do_skip;
next_state = do_reset;
+ if (!xattr_len)
+ return 0;
+
+ str[sizeof(hdr->c_size)] = 0;
+
+ while (buf < bufend) {
+ char *xattr_name, *xattr_value;
+ unsigned long xattr_entry_size, xattr_value_size;
+ int ret;
+
+ hdr = (struct xattr_hdr *)buf;
+ memcpy(str, hdr->c_size, sizeof(hdr->c_size));
+ ret = kstrtoul(str, 16, &xattr_entry_size);
+ buf += xattr_entry_size;
+ if (ret || buf > bufend) {
+ error("malformed xattrs");
+ break;
+ }
+
+ xattr_name = hdr->c_data;
+ xattr_value = xattr_name + strlen(xattr_name) + 1;
+ xattr_value_size = buf - xattr_value;
+
+ ret = sys_lsetxattr(name_buf, xattr_name, xattr_value,
+ xattr_value_size, 0);
+ pr_debug("%s: %s size: %lu val: %s (ret: %d)\n", name_buf,
+ xattr_name, xattr_value_size, xattr_value, ret);
+ }
return 0;
}

--
2.10.3.dirty


2018-02-16 20:38:53

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 12/15] gen_init_cpio: set extended attributes for newcx format

gen_init_cpio creates CPIO archive according to cpio_list manifest file
that contains list of archive entries (one per line). To be able to
store extended attributes in newcx CPIO format we need to pass them via
cpio_list file.

One way of doing it would be to append xattrs to each entry line, but
"file" lines have a variable number of elements because of hardlinks. It
is not obvious how to mark end of hardlinks and start of xattrs in this
case.

This patch introduces a new entry type: "xattr". Each "xattr" line
specify one name=value pair. xattr values are applied to the next
non-xattr line. There can be multiple "xattr" lines before non-xattr
line.

It may be more logical to have xattr lines after corresponding
file entry, but it makes parsing a bit more complex and needs more
intrusive changes.

Xattr value is hex-encoded (see getfattr(1)). Plain string variant would
be easier to read, but special symbols have to be escaped. Hex encoding
is much simpler.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
usr/gen_init_cpio.c | 144 +++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 121 insertions(+), 23 deletions(-)

diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index 25afd5b4af77..964f300620ca 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -10,6 +10,8 @@
#include <errno.h>
#include <ctype.h>
#include <limits.h>
+#include <sys/xattr.h>
+#include <assert.h>

/*
* Original work by Jeff Garzik
@@ -49,21 +51,10 @@ static void push_pad (void)
}
}

-static void push_rest(const char *name)
+static void push_string_padded(const char *name)
{
- unsigned int name_len = strlen(name) + 1;
- unsigned int tmp_ofs;
-
- fputs(name, stdout);
- putchar(0);
- offset += name_len;
-
- tmp_ofs = name_len + cpio_hdr_size;
- while (tmp_ofs & 3) {
- putchar(0);
- offset++;
- tmp_ofs++;
- }
+ push_string(name);
+ push_pad();
}

struct cpio_header {
@@ -124,6 +115,7 @@ static void push_hdr(const struct cpio_header *hdr)
hdr->check);
}
fputs(s, stdout);
+ assert((offset & 3) == 0);
offset += cpio_hdr_size;
}

@@ -136,7 +128,7 @@ static void cpio_trailer(void)
};

push_hdr(&hdr);
- push_rest(name);
+ push_string_padded(name);

while (offset % 512) {
putchar(0);
@@ -144,6 +136,96 @@ static void cpio_trailer(void)
}
}

+struct xattr_hdr {
+ char c_size[8]; /* total size including c_size field */
+ char c_data[];
+};
+static unsigned int xattr_buflen;
+static char xattr_buf[4096];
+
+static void push_xattrs(void)
+{
+ if (!newcx || !xattr_buflen)
+ return;
+
+ if (fwrite(xattr_buf, xattr_buflen, 1, stdout) != 1)
+ fprintf(stderr, "writing xattrs failed\n");
+ offset += xattr_buflen;
+ xattr_buflen = 0;
+
+ push_pad();
+}
+
+static int convert_hex_string(const char *hex_str, char *out, size_t out_size)
+{
+ char buf[3];
+ size_t str_len = strlen(hex_str);
+
+ if (str_len % 2 != 0 || str_len / 2 > out_size)
+ return 0;
+
+ buf[2] = '\0';
+ while (*hex_str != '\0') {
+ buf[0] = *hex_str++;
+ buf[1] = *hex_str++;
+ *out++ = (char)strtol(buf, NULL, 16);
+ }
+
+ return str_len / 2;
+}
+
+static int collect_xattr(const char *line)
+{
+ const char *name, *value;
+ size_t name_len, value_len;
+ char *buf = xattr_buf + xattr_buflen;
+ struct xattr_hdr *hdr = (struct xattr_hdr *)buf;
+ char *bufend = xattr_buf + sizeof(xattr_buf);
+ char *value_buf;
+ size_t xattr_entry_size;
+ char size_str[sizeof(hdr->c_size) + 1];
+
+ if (!newcx)
+ return 0;
+
+ name = line;
+ value = strchr(line, '=');
+ if (!value) {
+ fprintf(stderr, "Unrecognized xattr format '%s'", line);
+ return -1;
+ }
+ name_len = value - name;
+ value++;
+
+ /*
+ * For now we support only hex encoded values.
+ * String or base64 can be added later.
+ */
+ if (strncmp(value, "0x", 2)) {
+ fprintf(stderr,
+ "Only hex encoded xattr value is supported '%s'",
+ value);
+ return -1;
+ }
+
+ value += 2;
+ value_buf = buf + sizeof(struct xattr_hdr) + name_len + 1;
+ value_len = convert_hex_string(value, value_buf, bufend - value_buf);
+ if (value_len == 0) {
+ fprintf(stderr, "Failed to parse xattr value '%s'", line);
+ return -1;
+ }
+ xattr_entry_size = sizeof(struct xattr_hdr) + name_len + 1 + value_len;
+
+ sprintf(size_str, "%08X", (unsigned int)xattr_entry_size);
+ memcpy(hdr->c_size, size_str, sizeof(hdr->c_size));
+ memcpy(hdr->c_data, name, name_len);
+ hdr->c_data[name_len] = '\0';
+ xattr_buflen += xattr_entry_size;
+
+ return 0;
+}
+
static int cpio_mkslink(const char *name, const char *target,
unsigned int mode, uid_t uid, gid_t gid)
{
@@ -160,12 +242,12 @@ static int cpio_mkslink(const char *name, const char *target,
.devmajor = 3,
.devminor = 1,
.namesize = strlen(name)+1,
+ .xattrsize = xattr_buflen,
};
push_hdr(&hdr);
- push_string(name);
- push_pad();
- push_string(target);
- push_pad();
+ push_string_padded(name);
+ push_xattrs();
+ push_string_padded(target);
return 0;
}

@@ -202,9 +284,11 @@ static int cpio_mkgeneric(const char *name, unsigned int mode,
.devmajor = 3,
.devminor = 1,
.namesize = strlen(name)+1,
+ .xattrsize = xattr_buflen,
};
push_hdr(&hdr);
- push_rest(name);
+ push_string_padded(name);
+ push_xattrs();
return 0;
}

@@ -291,9 +375,11 @@ static int cpio_mknod(const char *name, unsigned int mode,
.rdevmajor = maj,
.rdevminor = min,
.namesize = strlen(name)+1,
+ .xattrsize = xattr_buflen,
};
push_hdr(&hdr);
- push_rest(name);
+ push_string_padded(name);
+ push_xattrs();
return 0;
}

@@ -376,10 +462,13 @@ static int cpio_mkfile(const char *name, const char *location,
.devmajor = 3,
.devminor = 1,
.namesize = namesize,
+ /* xattrs go on last link */
+ .xattrsize = (i == nlinks) ? xattr_buflen : 0,
};
push_hdr(&hdr);
- push_string(name);
- push_pad();
+ push_string_padded(name);
+ if (hdr.xattrsize)
+ push_xattrs();

if (size) {
if (fwrite(filebuf, size, 1, stdout) != 1) {
@@ -485,6 +574,8 @@ static void usage(const char *prog)
"slink <name> <target> <mode> <uid> <gid>\n"
"pipe <name> <mode> <uid> <gid>\n"
"sock <name> <mode> <uid> <gid>\n"
+ "# xattr line is applied to the next non-xattr entry\n"
+ "xattr <xattr_name>=<xattr_val>\n"
"\n"
"<name> name of the file/dir/nod/etc in the archive\n"
"<location> location of the file in the current filesystem\n"
@@ -497,12 +588,16 @@ static void usage(const char *prog)
"<maj> major number of nod\n"
"<min> minor number of nod\n"
"<hard links> space separated list of other links to file\n"
+ "<xattr_name> extended attribute name\n"
+ "<xattr_val> hex-encoded extended attribute value\n"
"\n"
"example:\n"
"# A simple initramfs\n"
"dir /dev 0755 0 0\n"
"nod /dev/console 0600 0 0 c 5 1\n"
"dir /root 0700 0 0\n"
+ "# set SELinux label 'system_u:object_r:bin_t:s0' for /sbin directory\n"
+ "xattr security.selinux=0x73797374656d5f753a6f626a6563745f723a62696e5f743a733000\n"
"dir /sbin 0755 0 0\n"
"file /sbin/kinit /usr/src/klibc/kinit/kinit 0755 0 0\n"
"\n"
@@ -532,6 +627,9 @@ struct file_handler file_handler_table[] = {
.type = "sock",
.handler = cpio_mksock_line,
}, {
+ .type = "xattr",
+ .handler = collect_xattr,
+ }, {
.type = NULL,
.handler = NULL,
}
--
2.10.3.dirty


2018-02-16 20:39:01

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 03/15] initramfs: store file name in name_buf

There is already name_buf buffer pre-allocated for a file name. No need
to allocate vcollected for every file. More over a name can be already
stored in name_buf by read_info() function.

Add memcpy_optional() function to handle such case.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 49cd2681a26f..b6ee675e5cdb 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -210,7 +210,6 @@ static inline void __init eat(unsigned n)
byte_count -= n;
}

-static __initdata char *vcollected;
static __initdata char *collected;
static long remains __initdata;
static __initdata char *collect;
@@ -324,6 +323,13 @@ static void __init clean_path(char *path, umode_t fmode)
}
}

+static void *memcpy_optional(void *dest, const void *src, size_t n)
+{
+ if (dest != src)
+ return memcpy(dest, src, n);
+ return dest;
+}
+
static __initdata int wfd;

static int __init do_name(void)
@@ -348,7 +354,8 @@ static int __init do_name(void)
sys_fchmod(wfd, mode);
if (body_len)
sys_ftruncate(wfd, body_len);
- vcollected = kstrdup(collected, GFP_KERNEL);
+ memcpy_optional(name_buf, collected,
+ N_ALIGN(name_len));
state = do_copy;
}
}
@@ -375,8 +382,7 @@ static int __init do_copy(void)
if (xwrite(wfd, victim, body_len) != body_len)
error("write error");
sys_close(wfd);
- do_utime(vcollected, mtime);
- kfree(vcollected);
+ do_utime(name_buf, mtime);
eat(body_len);
state = do_skip;
return 0;
--
2.10.3.dirty


2018-02-16 20:39:24

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 07/15] initramfs: split header layout information from parsing function

Header parsing has hardcoded assumption about header field size and
layout. It is hard to modify the function to parse a new format.

Move information about size and layout into a data structure to
make parsing code more generic and simplify adding a new format.
This also removes some magic numbers.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 122 +++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 92 insertions(+), 30 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index b3d39c8793be..7f0bbfde94e3 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -150,39 +150,100 @@ static void __init dir_utime(void)
}
}

-static __initdata time64_t mtime;
-
/* cpio header parsing */
-
-static __initdata unsigned long ino, major, minor, nlink;
+static __initdata time64_t mtime;
+static __initdata u32 ino, major, minor, nlink, rmajor, rminor;
static __initdata umode_t mode;
-static __initdata unsigned long body_len, name_len;
+static __initdata u32 body_len, name_len;
static __initdata uid_t uid;
static __initdata gid_t gid;
-static __initdata unsigned rdev;
+static __initdata u32 mode_u32;
+
+struct cpio_hdr_field {
+ size_t offset;
+ size_t size;
+ void *out;
+ size_t out_size;
+ const char *name;
+};
+
+#define HDR_FIELD(type, field, variable) \
+ { .offset = offsetof(type, field) + \
+ BUILD_BUG_ON_ZERO(sizeof(*(variable))*2 < FIELD_SIZEOF(type, field)),\
+ .size = FIELD_SIZEOF(type, field), \
+ .out = variable, \
+ .out_size = sizeof(*(variable)), \
+ .name = #field }
+
+#define NEWC_FIELD(field, variable) \
+ HDR_FIELD(struct cpio_newc_header, field, variable)
+
+#define CPIO_MAX_HEADER_SIZE sizeof(struct cpio_newc_header)
+#define CPIO_MAX_FIELD_SIZE 8
+#define CPIO_MAGIC_SIZE 6
+
+struct cpio_newc_header {
+ char c_ino[8];
+ char c_mode[8];
+ char c_uid[8];
+ char c_gid[8];
+ char c_nlink[8];
+ char c_mtime[8];
+ char c_filesize[8];
+ char c_devmajor[8];
+ char c_devminor[8];
+ char c_rdevmajor[8];
+ char c_rdevminor[8];
+ char c_namesize[8];
+ char c_check[8];
+};
+
+static struct cpio_hdr_field cpio_newc_header_info[] __initdata = {
+ NEWC_FIELD(c_ino, &ino),
+ NEWC_FIELD(c_mode, &mode_u32),
+ NEWC_FIELD(c_uid, &uid),
+ NEWC_FIELD(c_gid, &gid),
+ NEWC_FIELD(c_nlink, &nlink),
+ NEWC_FIELD(c_mtime, &mtime),
+ NEWC_FIELD(c_filesize, &body_len),
+ NEWC_FIELD(c_devmajor, &major),
+ NEWC_FIELD(c_devminor, &minor),
+ NEWC_FIELD(c_rdevmajor, &rmajor),
+ NEWC_FIELD(c_rdevminor, &rminor),
+ NEWC_FIELD(c_namesize, &name_len),
+ { 0 },
+};

static void __init parse_header(char *s)
{
- unsigned long parsed[12];
- char buf[9];
- int i;
-
- buf[8] = '\0';
- for (i = 0; i < 12; i++, s += 8) {
- memcpy(buf, s, 8);
- parsed[i] = simple_strtoul(buf, NULL, 16);
+ char buf[CPIO_MAX_FIELD_SIZE + 1];
+ struct cpio_hdr_field *field = cpio_newc_header_info;
+
+ while (field->size) {
+ int ret = 0;
+
+ memcpy(buf, s + field->offset, field->size);
+ buf[field->size] = '\0';
+ switch (field->out_size) {
+ case sizeof(u32):
+ ret = kstrtou32(buf, 16, field->out);
+ pr_debug("cpio field %s: %u, buf: %s\n",
+ field->name, *(u32 *)field->out, buf);
+ break;
+ case sizeof(u64):
+ ret = kstrtou64(buf, 16, field->out);
+ pr_debug("cpio field %s: %llu, buf: %s\n",
+ field->name, *(u64 *)field->out, buf);
+ break;
+ default:
+ BUG_ON(1);
+ }
+
+ if (ret)
+ pr_err("invalid cpio header field (%d)", ret);
+ field++;
}
- ino = parsed[0];
- mode = parsed[1];
- uid = parsed[2];
- gid = parsed[3];
- nlink = parsed[4];
- mtime = parsed[5]; /* breaks in y2106 */
- body_len = parsed[6];
- major = parsed[7];
- minor = parsed[8];
- rdev = new_encode_dev(MKDEV(parsed[9], parsed[10]));
- name_len = parsed[11];
+ mode = mode_u32;
}

/* FSM */
@@ -234,7 +295,7 @@ static __initdata char *header_buf, *symlink_buf, *name_buf;

static int __init do_start(void)
{
- read_into(header_buf, 6, do_format);
+ read_into(header_buf, CPIO_MAGIC_SIZE, do_format);
return 0;
}

@@ -254,15 +315,15 @@ static int __init do_collect(void)

static int __init do_format(void)
{
- if (memcmp(collected, "070707", 6)==0) {
+ if (memcmp(collected, "070707", CPIO_MAGIC_SIZE) == 0) {
error("incorrect cpio method used: use -H newc option");
return 1;
}
- if (memcmp(collected, "070701", 6)) {
+ if (memcmp(collected, "070701", CPIO_MAGIC_SIZE)) {
error("no cpio magic");
return 1;
}
- read_into(header_buf, 104, do_header);
+ read_into(header_buf, sizeof(struct cpio_newc_header), do_header);
return 0;
}

@@ -374,6 +435,7 @@ static int __init do_create(void)
} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
S_ISFIFO(mode) || S_ISSOCK(mode)) {
if (maybe_link(name_buf) == 0) {
+ u32 rdev = new_encode_dev(MKDEV(rmajor, rminor));
sys_mknod(name_buf, mode, rdev);
sys_chown(name_buf, uid, gid);
sys_chmod(name_buf, mode);
@@ -464,7 +526,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
const char *compress_name;
static __initdata char msg_buf[64];

- header_buf = kmalloc(104, GFP_KERNEL);
+ header_buf = kmalloc(CPIO_MAX_HEADER_SIZE, GFP_KERNEL);
symlink_buf = kmalloc(PATH_MAX + 1, GFP_KERNEL);
name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);

--
2.10.3.dirty


2018-02-16 20:39:32

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 06/15] initramfs: separate reading cpio method from header

From: Mimi Zohar <[email protected]>

In preparation for adding xattr support, read the CPIO method
separately from the rest of the header.

Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 2d5920c094e0..b3d39c8793be 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -168,7 +168,7 @@ static void __init parse_header(char *s)
int i;

buf[8] = '\0';
- for (i = 0, s += 6; i < 12; i++, s += 8) {
+ for (i = 0; i < 12; i++, s += 8) {
memcpy(buf, s, 8);
parsed[i] = simple_strtoul(buf, NULL, 16);
}
@@ -189,6 +189,7 @@ static void __init parse_header(char *s)

static int __init do_start(void);
static int __init do_collect(void);
+static int __init do_format(void);
static int __init do_header(void);
static int __init do_skip(void);
static int __init do_name(void);
@@ -233,7 +234,7 @@ static __initdata char *header_buf, *symlink_buf, *name_buf;

static int __init do_start(void)
{
- read_into(header_buf, 110, do_header);
+ read_into(header_buf, 6, do_format);
return 0;
}

@@ -251,7 +252,7 @@ static int __init do_collect(void)
return 0;
}

-static int __init do_header(void)
+static int __init do_format(void)
{
if (memcmp(collected, "070707", 6)==0) {
error("incorrect cpio method used: use -H newc option");
@@ -261,6 +262,12 @@ static int __init do_header(void)
error("no cpio magic");
return 1;
}
+ read_into(header_buf, 104, do_header);
+ return 0;
+}
+
+static int __init do_header(void)
+{
parse_header(collected);
next_header = this_header + N_ALIGN(name_len) + body_len;
next_header = (next_header + 3) & ~3;
@@ -457,7 +464,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
const char *compress_name;
static __initdata char msg_buf[64];

- header_buf = kmalloc(110, GFP_KERNEL);
+ header_buf = kmalloc(104, GFP_KERNEL);
symlink_buf = kmalloc(PATH_MAX + 1, GFP_KERNEL);
name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);

--
2.10.3.dirty


2018-02-16 20:39:45

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 01/15] Documentation: add newcx initramfs format description

Many of the Linux security/integrity features are dependent on file
metadata, stored as extended attributes (xattrs), for making decisions.
These features need to be initialized during initcall and enabled as
early as possible for complete security coverage.

Initramfs (tmpfs) supports xattrs, but newc CPIO archive format does not
support including them into the archive.

This patch describes "extended" newc format (newcx) that is based on
newc and has following changes:
- extended attributes support
- increased size of filesize to support files >4GB
- increased mtime field size to have 64 bits of seconds and added a
field for nanoseconds
- removed unused checksum field

Signed-off-by: Taras Kondratiuk <[email protected]>
Signed-off-by: Mimi Zohar <[email protected]>
Signed-off-by: Victor Kamensky <[email protected]>
---
Documentation/early-userspace/buffer-format.txt | 46 ++++++++++++++++++++++---
1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/Documentation/early-userspace/buffer-format.txt b/Documentation/early-userspace/buffer-format.txt
index e1fd7f9dad16..8349b2f28b4b 100644
--- a/Documentation/early-userspace/buffer-format.txt
+++ b/Documentation/early-userspace/buffer-format.txt
@@ -24,6 +24,7 @@ grammar, where:
+ indicates concatenation
GZIP() indicates the gzip(1) of the operand
ALGN(n) means padding with null bytes to an n-byte boundary
+ [n] means size of field is n bytes

initramfs := ("\0" | cpio_archive | cpio_gzip_archive)*

@@ -31,20 +32,33 @@ grammar, where:

cpio_archive := cpio_file* + (<nothing> | cpio_trailer)

- cpio_file := ALGN(4) + cpio_header + filename + "\0" + ALGN(4) + data
+ cpio_file := (cpio_newc_file | cpio_newcx_file)
+
+ cpio_newc_file := ALGN(4) + cpio_newc_header + filename + "\0" + \
+ ALGN(4) + data
+
+ cpio_newcx_file := ALGN(4) + cpio_newcx_header + filename + "\0" + \
+ ALGN(4) + xattrs + ALGN(4) + data
+
+ xattrs := xattr_entry*
+
+ xattr_entry := xattr_size[8] + xattr_name + "\0" + xattr_value

cpio_trailer := ALGN(4) + cpio_header + "TRAILER!!!\0" + ALGN(4)


In human terms, the initramfs buffer contains a collection of
-compressed and/or uncompressed cpio archives (in the "newc" or "crc"
-formats); arbitrary amounts zero bytes (for padding) can be added
-between members.
+compressed and/or uncompressed cpio archives; arbitrary amounts
+zero bytes (for padding) can be added between members.

The cpio "TRAILER!!!" entry (cpio end-of-archive) is optional, but is
not ignored; see "handling of hard links" below.

-The structure of the cpio_header is as follows (all fields contain
+xattr_size is a total size of xattr_entry including 8 bytes of
+xattr_size. xattr_size has the same hexadecimal ASCII encoding as other
+fields of cpio header (see below).
+
+The structure of the cpio_newc_header is as follows (all fields contain
hexadecimal ASCII numbers fully padded with '0' on the left to the
full width of the field, for example, the integer 4780 is represented
by the ASCII string "000012ac"):
@@ -81,6 +95,28 @@ algorithm used.
If the filename is "TRAILER!!!" this is actually an end-of-archive
marker; the c_filesize for an end-of-archive marker must be zero.

+"Extended" newc format (newcx)
+"newcx" cpio format extends "newc" by increasing size of some fields
+and adding extended attributes support. cpio_newcx_header structure:
+
+Field name Field size Meaning
+c_magic 6 bytes The string "070703"
+c_ino 8 bytes File inode number
+c_mode 8 bytes File mode and permissions
+c_uid 8 bytes File uid
+c_gid 8 bytes File gid
+c_nlink 8 bytes Number of links
+c_mtime 16 bytes Modification time (seconds)
+c_mtime_nsec 8 bytes Modification time (nanoseconds)
+c_filesize 16 bytes Size of data field
+c_maj 8 bytes Major part of file device number
+c_min 8 bytes Minor part of file device number
+c_rmaj 8 bytes Major part of device node reference
+c_rmin 8 bytes Minor part of device node reference
+c_namesize 8 bytes Length of filename, including final \0
+c_xattrs_size 8 bytes Size of xattrs field
+
+Most of the fields match cpio_newc_header. c_chksum field is dropped.

*** Handling of hard links

--
2.10.3.dirty


2018-02-16 20:40:04

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 05/15] initramfs: move files creation into separate state

Move most of the file creation logic into a separate state. This splits
collection of data stage from data processing and makes it easier to add
additional states for a new archive format.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 52 ++++++++++++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index d0ab7ad6ac05..2d5920c094e0 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -192,6 +192,7 @@ static int __init do_collect(void);
static int __init do_header(void);
static int __init do_skip(void);
static int __init do_name(void);
+static int __init do_create(void);
static int __init do_copy(void);
static int __init do_symlink(void);
static int __init do_reset(void);
@@ -292,12 +293,12 @@ static int __init do_reset(void)
return 1;
}

-static int __init maybe_link(void)
+static int __init maybe_link(char *name)
{
if (nlink >= 2) {
- char *old = find_link(major, minor, ino, mode, collected);
+ char *old = find_link(major, minor, ino, mode, name);
if (old)
- return (sys_link(old, collected) < 0) ? -1 : 1;
+ return (sys_link(old, name) < 0) ? -1 : 1;
}
return 0;
}
@@ -321,52 +322,59 @@ static void *memcpy_optional(void *dest, const void *src, size_t n)
return dest;
}

-static __initdata int wfd;
-
static int __init do_name(void)
{
- state = do_skip;
- next_state = do_reset;
if (strcmp(collected, "TRAILER!!!") == 0) {
+ state = do_skip;
+ next_state = do_reset;
free_hash();
return 0;
}
- clean_path(collected, mode);
+ memcpy_optional(name_buf, collected, N_ALIGN(name_len));
+ state = do_create;
+ return 0;
+}
+
+
+static __initdata int wfd;
+
+static int __init do_create(void)
+{
+ state = do_skip;
+ next_state = do_reset;
+ clean_path(name_buf, mode);
if (S_ISREG(mode)) {
- int ml = maybe_link();
+ int ml = maybe_link(name_buf);
if (ml >= 0) {
int openflags = O_WRONLY|O_CREAT;
if (ml != 1)
openflags |= O_TRUNC;
- wfd = sys_open(collected, openflags, mode);
+ wfd = sys_open(name_buf, openflags, mode);

if (wfd >= 0) {
sys_fchown(wfd, uid, gid);
sys_fchmod(wfd, mode);
if (body_len)
sys_ftruncate(wfd, body_len);
- memcpy_optional(name_buf, collected,
- N_ALIGN(name_len));
state = do_copy;
}
}
} else if (S_ISDIR(mode)) {
- sys_mkdir(collected, mode);
- sys_chown(collected, uid, gid);
- sys_chmod(collected, mode);
- dir_add(collected, mtime);
+ sys_mkdir(name_buf, mode);
+ sys_chown(name_buf, uid, gid);
+ sys_chmod(name_buf, mode);
+ dir_add(name_buf, mtime);
} else if (S_ISBLK(mode) || S_ISCHR(mode) ||
S_ISFIFO(mode) || S_ISSOCK(mode)) {
- if (maybe_link() == 0) {
- sys_mknod(collected, mode, rdev);
- sys_chown(collected, uid, gid);
- sys_chmod(collected, mode);
- do_utime(collected, mtime);
+ if (maybe_link(name_buf) == 0) {
+ sys_mknod(name_buf, mode, rdev);
+ sys_chown(name_buf, uid, gid);
+ sys_chmod(name_buf, mode);
+ do_utime(name_buf, mtime);
}
} else if (S_ISLNK(mode)) {
if (body_len > PATH_MAX)
return 0;
- memcpy_optional(name_buf, collected, N_ALIGN(name_len));
read_into(symlink_buf, body_len, do_symlink);
}
return 0;
--
2.10.3.dirty


2018-02-16 20:40:54

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 02/15] initramfs: replace states with function pointers

Currently the FSM states are mapped directly to function pointers. Extra
level of intirection is not needed and makes navigation over the code
harder. One can't jump between states directly when browsing code (e.g.
with cscope). Need to go through actions[] array each time.

Replace states with their action function pointers. No behaviour change.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 73 +++++++++++++++++++++++++-------------------------------
1 file changed, 32 insertions(+), 41 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index 7e99a0038942..49cd2681a26f 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -187,16 +187,17 @@ static void __init parse_header(char *s)

/* FSM */

-static __initdata enum state {
- Start,
- Collect,
- GotHeader,
- SkipIt,
- GotName,
- CopyFile,
- GotSymlink,
- Reset
-} state, next_state;
+static int __init do_start(void);
+static int __init do_collect(void);
+static int __init do_header(void);
+static int __init do_skip(void);
+static int __init do_name(void);
+static int __init do_copy(void);
+static int __init do_symlink(void);
+static int __init do_reset(void);
+
+typedef int (*fsm_state_t)(void);
+static __initdata fsm_state_t state, next_state;

static __initdata char *victim;
static unsigned long byte_count __initdata;
@@ -214,7 +215,7 @@ static __initdata char *collected;
static long remains __initdata;
static __initdata char *collect;

-static void __init read_into(char *buf, unsigned size, enum state next)
+static void __init read_into(char *buf, unsigned size, fsm_state_t next)
{
if (byte_count >= size) {
collected = victim;
@@ -224,7 +225,7 @@ static void __init read_into(char *buf, unsigned size, enum state next)
collect = collected = buf;
remains = size;
next_state = next;
- state = Collect;
+ state = do_collect;
}
}

@@ -232,7 +233,7 @@ static __initdata char *header_buf, *symlink_buf, *name_buf;

static int __init do_start(void)
{
- read_into(header_buf, 110, GotHeader);
+ read_into(header_buf, 110, do_header);
return 0;
}

@@ -263,7 +264,7 @@ static int __init do_header(void)
parse_header(collected);
next_header = this_header + N_ALIGN(name_len) + body_len;
next_header = (next_header + 3) & ~3;
- state = SkipIt;
+ state = do_skip;
if (name_len <= 0 || name_len > PATH_MAX)
return 0;
if (S_ISLNK(mode)) {
@@ -271,12 +272,12 @@ static int __init do_header(void)
return 0;
collect = collected = symlink_buf;
remains = N_ALIGN(name_len) + body_len;
- next_state = GotSymlink;
- state = Collect;
+ next_state = do_symlink;
+ state = do_collect;
return 0;
}
if (S_ISREG(mode) || !body_len)
- read_into(name_buf, N_ALIGN(name_len), GotName);
+ read_into(name_buf, N_ALIGN(name_len), do_name);
return 0;
}

@@ -327,8 +328,8 @@ static __initdata int wfd;

static int __init do_name(void)
{
- state = SkipIt;
- next_state = Reset;
+ state = do_skip;
+ next_state = do_reset;
if (strcmp(collected, "TRAILER!!!") == 0) {
free_hash();
return 0;
@@ -348,7 +349,7 @@ static int __init do_name(void)
if (body_len)
sys_ftruncate(wfd, body_len);
vcollected = kstrdup(collected, GFP_KERNEL);
- state = CopyFile;
+ state = do_copy;
}
}
} else if (S_ISDIR(mode)) {
@@ -377,7 +378,7 @@ static int __init do_copy(void)
do_utime(vcollected, mtime);
kfree(vcollected);
eat(body_len);
- state = SkipIt;
+ state = do_skip;
return 0;
} else {
if (xwrite(wfd, victim, byte_count) != byte_count)
@@ -395,29 +396,19 @@ static int __init do_symlink(void)
sys_symlink(collected + N_ALIGN(name_len), collected);
sys_lchown(collected, uid, gid);
do_utime(collected, mtime);
- state = SkipIt;
- next_state = Reset;
+ state = do_skip;
+ next_state = do_reset;
return 0;
}

-static __initdata int (*actions[])(void) = {
- [Start] = do_start,
- [Collect] = do_collect,
- [GotHeader] = do_header,
- [SkipIt] = do_skip,
- [GotName] = do_name,
- [CopyFile] = do_copy,
- [GotSymlink] = do_symlink,
- [Reset] = do_reset,
-};
-
static long __init write_buffer(char *buf, unsigned long len)
{
byte_count = len;
victim = buf;

- while (!actions[state]())
- ;
+ do
+ pr_debug("state: %pf\n", state);
+ while (!state());
return len - byte_count;
}

@@ -433,11 +424,11 @@ static long __init flush_buffer(void *bufv, unsigned long len)
if (c == '0') {
buf += written;
len -= written;
- state = Start;
+ state = do_start;
} else if (c == 0) {
buf += written;
len -= written;
- state = Reset;
+ state = do_reset;
} else
error("junk in compressed archive");
}
@@ -462,13 +453,13 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
if (!header_buf || !symlink_buf || !name_buf)
panic("can't allocate buffers");

- state = Start;
+ state = do_start;
this_header = 0;
message = NULL;
while (!message && len) {
loff_t saved_offset = this_header;
if (*buf == '0' && !(this_header & 3)) {
- state = Start;
+ state = do_start;
written = write_buffer(buf, len);
buf += written;
len -= written;
@@ -497,7 +488,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
}
} else
error("junk in compressed archive");
- if (state != Reset)
+ if (state != do_reset)
error("junk in compressed archive");
this_header = saved_offset + my_inptr;
buf += my_inptr;
--
2.10.3.dirty


2018-02-16 20:41:26

by Taras Kondratiuk

[permalink] [raw]
Subject: [PATCH v3 04/15] initramfs: remove unnecessary symlinks processing shortcut

Special handling of symlinks in do_header() assumes that name and body
entries are sequential and reads them together. This shortcut has no
real performance benefits, but it complicates changes to the state
machine.

Make handling of symlinks more similar to a regular files. Store name
in name_buf and destination in symlink_buf.

Signed-off-by: Taras Kondratiuk <[email protected]>
---
init/initramfs.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index b6ee675e5cdb..d0ab7ad6ac05 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -266,16 +266,7 @@ static int __init do_header(void)
state = do_skip;
if (name_len <= 0 || name_len > PATH_MAX)
return 0;
- if (S_ISLNK(mode)) {
- if (body_len > PATH_MAX)
- return 0;
- collect = collected = symlink_buf;
- remains = N_ALIGN(name_len) + body_len;
- next_state = do_symlink;
- state = do_collect;
- return 0;
- }
- if (S_ISREG(mode) || !body_len)
+ if (S_ISREG(mode) || S_ISLNK(mode) || !body_len)
read_into(name_buf, N_ALIGN(name_len), do_name);
return 0;
}
@@ -372,6 +363,11 @@ static int __init do_name(void)
sys_chmod(collected, mode);
do_utime(collected, mtime);
}
+ } else if (S_ISLNK(mode)) {
+ if (body_len > PATH_MAX)
+ return 0;
+ memcpy_optional(name_buf, collected, N_ALIGN(name_len));
+ read_into(symlink_buf, body_len, do_symlink);
}
return 0;
}
@@ -397,11 +393,12 @@ static int __init do_copy(void)

static int __init do_symlink(void)
{
- collected[N_ALIGN(name_len) + body_len] = '\0';
- clean_path(collected, 0);
- sys_symlink(collected + N_ALIGN(name_len), collected);
- sys_lchown(collected, uid, gid);
- do_utime(collected, mtime);
+ memcpy_optional(symlink_buf, collected, body_len);
+ symlink_buf[body_len] = '\0';
+ clean_path(name_buf, 0);
+ sys_symlink(symlink_buf, name_buf);
+ sys_lchown(name_buf, uid, gid);
+ do_utime(name_buf, mtime);
state = do_skip;
next_state = do_reset;
return 0;
@@ -453,7 +450,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)
static __initdata char msg_buf[64];

header_buf = kmalloc(110, GFP_KERNEL);
- symlink_buf = kmalloc(PATH_MAX + N_ALIGN(PATH_MAX) + 1, GFP_KERNEL);
+ symlink_buf = kmalloc(PATH_MAX + 1, GFP_KERNEL);
name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);

if (!header_buf || !symlink_buf || !name_buf)
--
2.10.3.dirty


2018-02-16 21:26:52

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description


On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
> On 02/16/18 12:33, Taras Kondratiuk wrote:
>> Many of the Linux security/integrity features are dependent on file
>> metadata, stored as extended attributes (xattrs), for making decisions.
>> These features need to be initialized during initcall and enabled as
>> early as possible for complete security coverage.
>>
>> Initramfs (tmpfs) supports xattrs, but newc CPIO archive format does not
>> support including them into the archive.
>>
>> This patch describes "extended" newc format (newcx) that is based on
>> newc and has following changes:
>> - extended attributes support
>> - increased size of filesize to support files >4GB
>> - increased mtime field size to have 64 bits of seconds and added a
>> field for nanoseconds
>> - removed unused checksum field
>>
>
> If you are going to implement a new, non-backwards-compatible format,
> you shouldn't replicate the mistakes of the current format. Specifically:

So rather than make minimal changes to the existing format and continue to
support the existing format (sharing as much code as possible), you recommend
gratuitous aesthetic changes?

> 1. The use of ASCII-encoded fixed-length numbers is an idiotic legacy
> from an era before there were any portable way of dealing with numbers
> with prespecified endianness.

It lets encoders and decoders easily share code with the existing cpio format,
which we still intend to be able to read and write.

> If you are going to use ASCII, make them
> delimited so that they don't have fixed limits, or just use binary.

When it's gzipped this accomplishes what? (Other than being gratuitously
different from the previous iteration?)

> The cpio header isn't fixed size, so that argument goes away, in fact
> the only way to determine the end of the header is to scan forward.
>
> 2. Alignment sensitivity! Because there is no header length
> information, the above scan tells you where the header ends, but there
> is padding before the data, and the size of that padding is only defined
> by alignment.

Again, these are minimal changes to the existing cpio format. You're complaining
about _cpio_, and that the new stuff isn't _different_ enough from it.

> 3. Inband encoding of EOF: if you actually have a filename "TRAILER!!!"
> you have problems.

Been there, done that:

http://lkml.iu.edu/hypermail/linux/kernel/1801.3/01791.html

> But first, before you define a whole new format for which no tools exist
> (you will have to work with the maintainers of the GNU tools to add
> support)

No, he's been working with the maintainer of toybox to add support (for about a
year now), which gets him the Android command line. And the kernel has its own
built-in tool to generate cpio images anyway.

Why would anyone care what the GNU project thinks?

> you should see how complex it would be to support the POSIX
> tar/pax format,

That argument was had (at length) when initramfs went in over a decade ago.
There are links in Documentation/filesystems/ramfs-rootfs-initramfs.txt to the
mailing list entries about it.

> which already has all the features you are seeking, and
> by now is well-supported.

So... tar wasn't well-supported 15 years ago? (Hasn't the kernel source always
been distributed via tarball back since 0.0.1?)

You're suggesting having a whole second codepath that shares no code with the
existing cpio extractor. Are you suggesting abandoning support for the existing
initramfs.cpio.gz file format?

Rob

2018-02-16 21:58:12

by Victor Kamensky

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description



On Fri, 16 Feb 2018, Rob Landley wrote:

>
> On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
>> On 02/16/18 12:33, Taras Kondratiuk wrote:
>>> Many of the Linux security/integrity features are dependent on file
>>> metadata, stored as extended attributes (xattrs), for making decisions.
>>> These features need to be initialized during initcall and enabled as
>>> early as possible for complete security coverage.
>>>
>>> Initramfs (tmpfs) supports xattrs, but newc CPIO archive format does not
>>> support including them into the archive.
>>>
>>> This patch describes "extended" newc format (newcx) that is based on
>>> newc and has following changes:
>>> - extended attributes support
>>> - increased size of filesize to support files >4GB
>>> - increased mtime field size to have 64 bits of seconds and added a
>>> field for nanoseconds
>>> - removed unused checksum field
>>>
>>
>> If you are going to implement a new, non-backwards-compatible format,
>> you shouldn't replicate the mistakes of the current format. Specifically:
>
> So rather than make minimal changes to the existing format and continue to
> support the existing format (sharing as much code as possible), you recommend
> gratuitous aesthetic changes?
>
>> 1. The use of ASCII-encoded fixed-length numbers is an idiotic legacy
>> from an era before there were any portable way of dealing with numbers
>> with prespecified endianness.
>
> It lets encoders and decoders easily share code with the existing cpio format,
> which we still intend to be able to read and write.
>
>> If you are going to use ASCII, make them
>> delimited so that they don't have fixed limits, or just use binary.
>
> When it's gzipped this accomplishes what? (Other than being gratuitously
> different from the previous iteration?)
>
>> The cpio header isn't fixed size, so that argument goes away, in fact
>> the only way to determine the end of the header is to scan forward.
>>
>> 2. Alignment sensitivity! Because there is no header length
>> information, the above scan tells you where the header ends, but there
>> is padding before the data, and the size of that padding is only defined
>> by alignment.
>
> Again, these are minimal changes to the existing cpio format. You're complaining
> about _cpio_, and that the new stuff isn't _different_ enough from it.
>
>> 3. Inband encoding of EOF: if you actually have a filename "TRAILER!!!"
>> you have problems.
>
> Been there, done that:
>
> http://lkml.iu.edu/hypermail/linux/kernel/1801.3/01791.html
>
>> But first, before you define a whole new format for which no tools exist
>> (you will have to work with the maintainers of the GNU tools to add
>> support)
>
> No, he's been working with the maintainer of toybox to add support (for about a
> year now), which gets him the Android command line. And the kernel has its own
> built-in tool to generate cpio images anyway.
>
> Why would anyone care what the GNU project thinks?

In our internal use of this patch series we do use gnu cpio
to create initramfs.cpio.

And reference to gnu cpio patch that supports newcx format is
posted in description for this serieis:

https://raw.githubusercontent.com/victorkamensky/initramfs-xattrs-poky/rocko/meta/recipes-extended/cpio/cpio-2.12/cpio-xattrs.patch

Whether GNU cpio maintainers will accept it is different matter.
We will try, but we need to start somewhere and agree on
new format first.

Thanks,
Victor

>> you should see how complex it would be to support the POSIX
>> tar/pax format,
>
> That argument was had (at length) when initramfs went in over a decade ago.
> There are links in Documentation/filesystems/ramfs-rootfs-initramfs.txt to the
> mailing list entries about it.
>
>> which already has all the features you are seeking, and
>> by now is well-supported.
>
> So... tar wasn't well-supported 15 years ago? (Hasn't the kernel source always
> been distributed via tarball back since 0.0.1?)
>
> You're suggesting having a whole second codepath that shares no code with the
> existing cpio extractor. Are you suggesting abandoning support for the existing
> initramfs.cpio.gz file format?
>
> Rob
>

2018-02-16 22:06:06

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

On 02/16/18 12:33, Taras Kondratiuk wrote:
> Many of the Linux security/integrity features are dependent on file
> metadata, stored as extended attributes (xattrs), for making decisions.
> These features need to be initialized during initcall and enabled as
> early as possible for complete security coverage.
>
> Initramfs (tmpfs) supports xattrs, but newc CPIO archive format does not
> support including them into the archive.
>
> This patch describes "extended" newc format (newcx) that is based on
> newc and has following changes:
> - extended attributes support
> - increased size of filesize to support files >4GB
> - increased mtime field size to have 64 bits of seconds and added a
> field for nanoseconds
> - removed unused checksum field
>

If you are going to implement a new, non-backwards-compatible format,
you shouldn't replicate the mistakes of the current format. Specifically:

1. The use of ASCII-encoded fixed-length numbers is an idiotic legacy
from an era before there were any portable way of dealing with numbers
with prespecified endianness. If you are going to use ASCII, make them
delimited so that they don't have fixed limits, or just use binary.

The cpio header isn't fixed size, so that argument goes away, in fact
the only way to determine the end of the header is to scan forward.

2. Alignment sensitivity! Because there is no header length
information, the above scan tells you where the header ends, but there
is padding before the data, and the size of that padding is only defined
by alignment.

3. Inband encoding of EOF: if you actually have a filename "TRAILER!!!"
you have problems.

But first, before you define a whole new format for which no tools exist
(you will have to work with the maintainers of the GNU tools to add
support) you should see how complex it would be to support the POSIX
tar/pax format, which already has all the features you are seeking, and
by now is well-supported.

-hpa


2018-02-17 02:24:34

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

On February 16, 2018 1:47:35 PM PST, Victor Kamensky <[email protected]> wrote:
>
>
>On Fri, 16 Feb 2018, Rob Landley wrote:
>
>>
>> On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
>>> On 02/16/18 12:33, Taras Kondratiuk wrote:
>>>> Many of the Linux security/integrity features are dependent on file
>>>> metadata, stored as extended attributes (xattrs), for making
>decisions.
>>>> These features need to be initialized during initcall and enabled
>as
>>>> early as possible for complete security coverage.
>>>>
>>>> Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
>does not
>>>> support including them into the archive.
>>>>
>>>> This patch describes "extended" newc format (newcx) that is based
>on
>>>> newc and has following changes:
>>>> - extended attributes support
>>>> - increased size of filesize to support files >4GB
>>>> - increased mtime field size to have 64 bits of seconds and added a
>>>> field for nanoseconds
>>>> - removed unused checksum field
>>>>
>>>
>>> If you are going to implement a new, non-backwards-compatible
>format,
>>> you shouldn't replicate the mistakes of the current format.
>Specifically:
>>
>> So rather than make minimal changes to the existing format and
>continue to
>> support the existing format (sharing as much code as possible), you
>recommend
>> gratuitous aesthetic changes?
>>
>>> 1. The use of ASCII-encoded fixed-length numbers is an idiotic
>legacy
>>> from an era before there were any portable way of dealing with
>numbers
>>> with prespecified endianness.
>>
>> It lets encoders and decoders easily share code with the existing
>cpio format,
>> which we still intend to be able to read and write.
>>
>>> If you are going to use ASCII, make them
>>> delimited so that they don't have fixed limits, or just use binary.
>>
>> When it's gzipped this accomplishes what? (Other than being
>gratuitously
>> different from the previous iteration?)
>>
>>> The cpio header isn't fixed size, so that argument goes away, in
>fact
>>> the only way to determine the end of the header is to scan forward.
>>>
>>> 2. Alignment sensitivity! Because there is no header length
>>> information, the above scan tells you where the header ends, but
>there
>>> is padding before the data, and the size of that padding is only
>defined
>>> by alignment.
>>
>> Again, these are minimal changes to the existing cpio format. You're
>complaining
>> about _cpio_, and that the new stuff isn't _different_ enough from
>it.
>>
>>> 3. Inband encoding of EOF: if you actually have a filename
>"TRAILER!!!"
>>> you have problems.
>>
>> Been there, done that:
>>
>> http://lkml.iu.edu/hypermail/linux/kernel/1801.3/01791.html
>>
>>> But first, before you define a whole new format for which no tools
>exist
>>> (you will have to work with the maintainers of the GNU tools to add
>>> support)
>>
>> No, he's been working with the maintainer of toybox to add support
>(for about a
>> year now), which gets him the Android command line. And the kernel
>has its own
>> built-in tool to generate cpio images anyway.
>>
>> Why would anyone care what the GNU project thinks?
>
>In our internal use of this patch series we do use gnu cpio
>to create initramfs.cpio.
>
>And reference to gnu cpio patch that supports newcx format is
>posted in description for this serieis:
>
>https://raw.githubusercontent.com/victorkamensky/initramfs-xattrs-poky/rocko/meta/recipes-extended/cpio/cpio-2.12/cpio-xattrs.patch
>
>Whether GNU cpio maintainers will accept it is different matter.
>We will try, but we need to start somewhere and agree on
>new format first.
>
>Thanks,
>Victor
>
>>> you should see how complex it would be to support the POSIX
>>> tar/pax format,
>>
>> That argument was had (at length) when initramfs went in over a
>decade ago.
>> There are links in
>Documentation/filesystems/ramfs-rootfs-initramfs.txt to the
>> mailing list entries about it.
>>
>>> which already has all the features you are seeking, and
>>> by now is well-supported.
>>
>> So... tar wasn't well-supported 15 years ago? (Hasn't the kernel
>source always
>> been distributed via tarball back since 0.0.1?)
>>
>> You're suggesting having a whole second codepath that shares no code
>with the
>> existing cpio extractor. Are you suggesting abandoning support for
>the existing
>> initramfs.cpio.gz file format?
>>
>> Rob
>>

Introducing new, incompatible data formats is an inherently *very* costly operation; unfortunately many engineers don't seem to have a good grip of just *how* expensive it is (see "silly embedded nonsense hacks", "too little, too soon".)

Cpio itself is a great horror show of just how bad this gets: a bunch of minor tweaks without finding underlying design bugs resulting in a ton of mutually incompatible formats. "They are almost the same" doesn't help: they are still incompatible.

Introducing a new incompatible data format without strong justification is engineering malpractice. Doing it under the non-justification of expedience ("oh, we can share most of the code") is aggravated engineering malpractice.

It is entirely possible that the modern posix tar/pax format is too complex to be practical in this case – that would be justifying a new format. But then you are taking the fundamental cost of breakage, and then the new format definitely should not be replicating known defects of another format and without at least some thought about how to avoid it in the future.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2018-02-17 10:04:59

by Taras Kondratiuk

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

Quoting [email protected] (2018-02-16 16:00:36)
> On February 16, 2018 1:47:35 PM PST, Victor Kamensky <[email protected]> wrote:
> >
> >
> >On Fri, 16 Feb 2018, Rob Landley wrote:
> >
> >>
> >> On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
> >>> On 02/16/18 12:33, Taras Kondratiuk wrote:
> >>>> Many of the Linux security/integrity features are dependent on file
> >>>> metadata, stored as extended attributes (xattrs), for making
> >decisions.
> >>>> These features need to be initialized during initcall and enabled
> >as
> >>>> early as possible for complete security coverage.
> >>>>
> >>>> Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
> >does not
> >>>> support including them into the archive.
> >>>>
> >>>> This patch describes "extended" newc format (newcx) that is based
> >on
> >>>> newc and has following changes:
> >>>> - extended attributes support
> >>>> - increased size of filesize to support files >4GB
> >>>> - increased mtime field size to have 64 bits of seconds and added a
> >>>> field for nanoseconds
> >>>> - removed unused checksum field
> >>>>
> >>>
> >>> If you are going to implement a new, non-backwards-compatible
> >format,
> >>> you shouldn't replicate the mistakes of the current format.
> >Specifically:
> >>
> >> So rather than make minimal changes to the existing format and
> >continue to
> >> support the existing format (sharing as much code as possible), you
> >recommend
> >> gratuitous aesthetic changes?
> >>
> >>> 1. The use of ASCII-encoded fixed-length numbers is an idiotic
> >legacy
> >>> from an era before there were any portable way of dealing with
> >numbers
> >>> with prespecified endianness.
> >>
> >> It lets encoders and decoders easily share code with the existing
> >cpio format,
> >> which we still intend to be able to read and write.
> >>
> >>> If you are going to use ASCII, make them
> >>> delimited so that they don't have fixed limits, or just use binary.
> >>
> >> When it's gzipped this accomplishes what? (Other than being
> >gratuitously
> >> different from the previous iteration?)
> >>
> >>> The cpio header isn't fixed size, so that argument goes away, in
> >fact
> >>> the only way to determine the end of the header is to scan forward.
> >>>
> >>> 2. Alignment sensitivity! Because there is no header length
> >>> information, the above scan tells you where the header ends, but
> >there
> >>> is padding before the data, and the size of that padding is only
> >defined
> >>> by alignment.
> >>
> >> Again, these are minimal changes to the existing cpio format. You're
> >complaining
> >> about _cpio_, and that the new stuff isn't _different_ enough from
> >it.
> >>
> >>> 3. Inband encoding of EOF: if you actually have a filename
> >"TRAILER!!!"
> >>> you have problems.
> >>
> >> Been there, done that:
> >>
> >> http://lkml.iu.edu/hypermail/linux/kernel/1801.3/01791.html
> >>
> >>> But first, before you define a whole new format for which no tools
> >exist
> >>> (you will have to work with the maintainers of the GNU tools to add
> >>> support)
> >>
> >> No, he's been working with the maintainer of toybox to add support
> >(for about a
> >> year now), which gets him the Android command line. And the kernel
> >has its own
> >> built-in tool to generate cpio images anyway.
> >>
> >> Why would anyone care what the GNU project thinks?
> >
> >In our internal use of this patch series we do use gnu cpio
> >to create initramfs.cpio.
> >
> >And reference to gnu cpio patch that supports newcx format is
> >posted in description for this serieis:
> >
> >https://raw.githubusercontent.com/victorkamensky/initramfs-xattrs-poky/rocko/meta/recipes-extended/cpio/cpio-2.12/cpio-xattrs.patch
> >
> >Whether GNU cpio maintainers will accept it is different matter.
> >We will try, but we need to start somewhere and agree on
> >new format first.
> >
> >Thanks,
> >Victor
> >
> >>> you should see how complex it would be to support the POSIX
> >>> tar/pax format,
> >>
> >> That argument was had (at length) when initramfs went in over a
> >decade ago.
> >> There are links in
> >Documentation/filesystems/ramfs-rootfs-initramfs.txt to the
> >> mailing list entries about it.
> >>
> >>> which already has all the features you are seeking, and
> >>> by now is well-supported.
> >>
> >> So... tar wasn't well-supported 15 years ago? (Hasn't the kernel
> >source always
> >> been distributed via tarball back since 0.0.1?)
> >>
> >> You're suggesting having a whole second codepath that shares no code
> >with the
> >> existing cpio extractor. Are you suggesting abandoning support for
> >the existing
> >> initramfs.cpio.gz file format?
> >>
> >> Rob
> >>
>
> Introducing new, incompatible data formats is an inherently *very* costly operation; unfortunately many engineers don't seem to have a good grip of just *how* expensive it is (see "silly embedded nonsense hacks", "too little, too soon".)
>
> Cpio itself is a great horror show of just how bad this gets: a bunch of minor tweaks without finding underlying design bugs resulting in a ton of mutually incompatible formats. "They are almost the same" doesn't help: they are still incompatible.
>
> Introducing a new incompatible data format without strong justification is engineering malpractice. Doing it under the non-justification of expedience ("oh, we can share most of the code") is aggravated engineering malpractice.
>
> It is entirely possible that the modern posix tar/pax format is too complex to be practical in this case – that would be justifying a new format. But then you are taking the fundamental cost of breakage, and then the new format definitely should not be replicating known defects of another format and without at least some thought about how to avoid it in the future.

I do understand a cost of adding a new format and I'd be very happy not
to do it if there is a better option. I did consider using tar/pax, but
looks like it was already discussed in 2001 between you and Al Viro [1]
and tar was rejected.

My main tar concerns:
- ustar+pax header is *huge*. E.g. directory entry in archive: pax 1536
bytes vs cpio <200 bytes. Overall compressed initramfs size increase
is not significant though.
- pax is not a strict format. E.g. xattrs may be stored under different
names: SHCILY.xattr (GNU tar, star) vs LIBARCHIVE.xattr (libarchive).

I'm not sure which option is better. Adding tar to the kernel or adding
new cpio format into several tools (GNU cpio, libarchive, busybox,
toybox) will result in approximately the same amount of code.

It would be nice to get Al Viro's thoughts on this.

[1] https://web.archive.org/web/20060909041730/http://www.uwsg.iu.edu/hypermail/linux/kernel/0112.2/1638.html

2018-02-17 17:33:59

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

On 02/16/2018 06:00 PM, [email protected] wrote:
> Introducing new, incompatible data formats is an inherently *very*
> costly operation; unfortunately many engineers don't seem to have a good grip
> of just *how* expensive it is (see "silly embedded nonsense hacks", "too
> little, too soon".)

So your argument is we should use the _existing_ cpio format that supports xattrs?

You keep bringing up the embedded world as a thing you don't understand and is
thus bad. I remember when you dismissed "I would like to constrain my
cross-compiling dependencies to a minimal set" as a... what did you call it, a
silly academic exercise? (Googles...)

https://lkml.org/lkml/2008/2/15/548

> Cpio itself is a great horror show of just how bad this gets:

That's not what you said last time?

http://lkml.iu.edu/hypermail/linux/kernel/0112.2/1540.html

> Introducing a new incompatible data format without strong justification

Here's you suggesting a new format when initramfs first went in, because you
disliked _both_ tar and cpio:

http://lkml.iu.edu/hypermail/linux/kernel/0112.2/1587.html

Seriously, there is a "why cpio rather than tar" section of
https://www.kernel.org/doc/Documentation/filesystems/ramfs-rootfs-initramfs.txt
with links to the messages. (www.uwsg became lkml in the links, I should submit
a patch fixing that, it redirected 6 months ago...)

We've _had_ this argument already. You are not bringing up _new_ arguments.

This patch set is because people want xattrs in initramfs. I still don't
personally understand why they want this, but they do. We need to still support
the existing file format for the forseeable future, and we might as well fix
y2038 while we're there (treating it as unsigned buys us a lot of decades, but
as long as we're bumping the version number anyway...).

Otherwise it tries to be the minimal set of changes to get us there. (My first
stab at this was dealing with sparse files, but runs of zeroes gzip pretty well
and tmpfs could always make itself sparse after the fact...)

> Doing it under the non-justification of expedience ("oh, we can share most>
> of the code") is aggravated engineering malpractice.

Coming from the guy who added perl as a build dependency to every project he
maintained simultaneously (the linux kernel, your bootloader, klibc), that seems
a lot more like an opinion than an objective metric.

> It is entirely possible that the modern posix tar/pax format is too complex

In the link above you declared it too complex in 2001. Partly because the gnu
tar and pax formats aren't really the same format.

> to be practical in this case – that would be justifying a new format. But
> then you are taking the fundamental cost of breakage, and then the new format
> definitely should not be replicating known defects of another format and
> without at least some thought about how to avoid it in the future.

Didn't Linus flame more than one developer for ripping things out and replacing
them with a new untested thing rather than leaving a trail of breadcrumbs from a
known working thing to another known working thing? Or has the right way to do
it changed since the 2.5 development cycle?

Strangely the poor souls suffering under the burden of cpio to use initramfs
today haven't been screaming out their agony in a detectable way. (They're mad
the kernel doesn't give better feedback about why init failed to launch and it
either paniced or fell through to the fallback ROOT=, my patch to make
devtmpfs_mount work for initramfs was trying to fix the "you pointed the kernel
at a root filesystem directory which it cpio'd up but there was /dev/console in
it so your init has no stdin/stdout/stderr and dies immediately because of it"
problem. And the recent thread about "please don't add a third knob to make
initramfs be tmpfs instead of ramfs" was another corner case of that). And I
have half an INITRAMFS_VERBOSE patch around here somewhere to printk() a lot
more status (and I need to update the initramfs documentation I wrote to help
people have an easier time using it...)

But that's not about archive format. That's kernel userspace bringup being
persnickety. The silent majority you speak for on this archive format issue is
pretty darn silent.

Was this recorded as a problem for you before somebody suggested changing it? I
tend to be public about https://twitter.com/landley/status/964620648050982912
and collect links to other people's concerns when I notice...

Or is this just your opinion?

Rob

2018-02-18 00:19:09

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

On Fri, 2018-02-16 at 12:59 -0800, H. Peter Anvin wrote:
> On 02/16/18 12:33, Taras Kondratiuk wrote:
> > Many of the Linux security/integrity features are dependent on file
> > metadata, stored as extended attributes (xattrs), for making decisions.
> > These features need to be initialized during initcall and enabled as
> > early as possible for complete security coverage.
> >
> > Initramfs (tmpfs) supports xattrs, but newc CPIO archive format does not
> > support including them into the archive.
> >
> > This patch describes "extended" newc format (newcx) that is based on
> > newc and has following changes:
> > - extended attributes support
> > - increased size of filesize to support files >4GB
> > - increased mtime field size to have 64 bits of seconds and added a
> > field for nanoseconds
> > - removed unused checksum field
> >
>
> If you are going to implement a new, non-backwards-compatible format,
> you shouldn't replicate the mistakes of the current format. Specifically:
>
> 1. The use of ASCII-encoded fixed-length numbers is an idiotic legacy
> from an era before there were any portable way of dealing with numbers
> with prespecified endianness. If you are going to use ASCII, make them
> delimited so that they don't have fixed limits, or just use binary.
>
> The cpio header isn't fixed size, so that argument goes away, in fact
> the only way to determine the end of the header is to scan forward.
>
> 2. Alignment sensitivity! Because there is no header length
> information, the above scan tells you where the header ends, but there
> is padding before the data, and the size of that padding is only defined
> by alignment.
>
> 3. Inband encoding of EOF: if you actually have a filename "TRAILER!!!"
> you have problems.
>
> But first, before you define a whole new format for which no tools exist
> (you will have to work with the maintainers of the GNU tools to add
> support) you should see how complex it would be to support the POSIX
> tar/pax format, which already has all the features you are seeking, and
> by now is well-supported.

The discussion about including xattrs in the initramfs didn't start
yesterday.  It's been on the list of measurement/appraisal gaps that
need to be closed for years.  Initially I planned on using tar, but at
the 2014 Kernel Summit I spoke with Al at length.  At the time, he was
very clear that tar is unnecessarily overly complicated and
recommended extending CPIO.

I took his advice.  Unfortunately, as soon as I posted an initial
patch set to include xattrs in CPIO, all of the problems with CPIO had
to be addressed before defining a new CPIO number.  Unfortunately,
this wasn't the only measurement/appraisal gap that needed to be
addressed.  I've been working on closing other gaps.

I'm really happy that someone has taken the time to work on this.
 Instead of derailing their attempt of extending CPIO to include
xattrs, I'd appreciate your making constructive suggestions.

Mimi


2018-02-18 00:26:03

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

On February 17, 2018 4:15:12 PM PST, Mimi Zohar <[email protected]> wrote:
>On Fri, 2018-02-16 at 12:59 -0800, H. Peter Anvin wrote:
>> On 02/16/18 12:33, Taras Kondratiuk wrote:
>> > Many of the Linux security/integrity features are dependent on file
>> > metadata, stored as extended attributes (xattrs), for making
>decisions.
>> > These features need to be initialized during initcall and enabled
>as
>> > early as possible for complete security coverage.
>> >
>> > Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
>does not
>> > support including them into the archive.
>> >
>> > This patch describes "extended" newc format (newcx) that is based
>on
>> > newc and has following changes:
>> > - extended attributes support
>> > - increased size of filesize to support files >4GB
>> > - increased mtime field size to have 64 bits of seconds and added a
>> > field for nanoseconds
>> > - removed unused checksum field
>> >
>>
>> If you are going to implement a new, non-backwards-compatible format,
>> you shouldn't replicate the mistakes of the current format.
>Specifically:
>>
>> 1. The use of ASCII-encoded fixed-length numbers is an idiotic legacy
>> from an era before there were any portable way of dealing with
>numbers
>> with prespecified endianness. If you are going to use ASCII, make
>them
>> delimited so that they don't have fixed limits, or just use binary.
>>
>> The cpio header isn't fixed size, so that argument goes away, in fact
>> the only way to determine the end of the header is to scan forward.
>>
>> 2. Alignment sensitivity! Because there is no header length
>> information, the above scan tells you where the header ends, but
>there
>> is padding before the data, and the size of that padding is only
>defined
>> by alignment.
>>
>> 3. Inband encoding of EOF: if you actually have a filename
>"TRAILER!!!"
>> you have problems.
>>
>> But first, before you define a whole new format for which no tools
>exist
>> (you will have to work with the maintainers of the GNU tools to add
>> support) you should see how complex it would be to support the POSIX
>> tar/pax format, which already has all the features you are seeking,
>and
>> by now is well-supported.
>
>The discussion about including xattrs in the initramfs didn't start
>yesterday.  It's been on the list of measurement/appraisal gaps that
>need to be closed for years.  Initially I planned on using tar, but at
>the 2014 Kernel Summit I spoke with Al at length.  At the time, he was
>very clear that tar is unnecessarily overly complicated and
>recommended extending CPIO.
>
>I took his advice.  Unfortunately, as soon as I posted an initial
>patch set to include xattrs in CPIO, all of the problems with CPIO had
>to be addressed before defining a new CPIO number.  Unfortunately,
>this wasn't the only measurement/appraisal gap that needed to be
>addressed.  I've been working on closing other gaps.
>
>I'm really happy that someone has taken the time to work on this.
> Instead of derailing their attempt of extending CPIO to include
>xattrs, I'd appreciate your making constructive suggestions.
>
>Mimi

I'm not trying to derail anything, but I do want to see it done well if it is going to be done. I have several ideas already, but I may not have a chance to write them down properly until after the weekend due to family obligations.

The assessment of pax/tar is useful; it should be added to the patch documentation in a future set.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2018-02-18 00:27:59

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

On February 17, 2018 4:15:12 PM PST, Mimi Zohar <[email protected]> wrote:
>On Fri, 2018-02-16 at 12:59 -0800, H. Peter Anvin wrote:
>> On 02/16/18 12:33, Taras Kondratiuk wrote:
>> > Many of the Linux security/integrity features are dependent on file
>> > metadata, stored as extended attributes (xattrs), for making
>decisions.
>> > These features need to be initialized during initcall and enabled
>as
>> > early as possible for complete security coverage.
>> >
>> > Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
>does not
>> > support including them into the archive.
>> >
>> > This patch describes "extended" newc format (newcx) that is based
>on
>> > newc and has following changes:
>> > - extended attributes support
>> > - increased size of filesize to support files >4GB
>> > - increased mtime field size to have 64 bits of seconds and added a
>> > field for nanoseconds
>> > - removed unused checksum field
>> >
>>
>> If you are going to implement a new, non-backwards-compatible format,
>> you shouldn't replicate the mistakes of the current format.
>Specifically:
>>
>> 1. The use of ASCII-encoded fixed-length numbers is an idiotic legacy
>> from an era before there were any portable way of dealing with
>numbers
>> with prespecified endianness. If you are going to use ASCII, make
>them
>> delimited so that they don't have fixed limits, or just use binary.
>>
>> The cpio header isn't fixed size, so that argument goes away, in fact
>> the only way to determine the end of the header is to scan forward.
>>
>> 2. Alignment sensitivity! Because there is no header length
>> information, the above scan tells you where the header ends, but
>there
>> is padding before the data, and the size of that padding is only
>defined
>> by alignment.
>>
>> 3. Inband encoding of EOF: if you actually have a filename
>"TRAILER!!!"
>> you have problems.
>>
>> But first, before you define a whole new format for which no tools
>exist
>> (you will have to work with the maintainers of the GNU tools to add
>> support) you should see how complex it would be to support the POSIX
>> tar/pax format, which already has all the features you are seeking,
>and
>> by now is well-supported.
>
>The discussion about including xattrs in the initramfs didn't start
>yesterday.  It's been on the list of measurement/appraisal gaps that
>need to be closed for years.  Initially I planned on using tar, but at
>the 2014 Kernel Summit I spoke with Al at length.  At the time, he was
>very clear that tar is unnecessarily overly complicated and
>recommended extending CPIO.
>
>I took his advice.  Unfortunately, as soon as I posted an initial
>patch set to include xattrs in CPIO, all of the problems with CPIO had
>to be addressed before defining a new CPIO number.  Unfortunately,
>this wasn't the only measurement/appraisal gap that needed to be
>addressed.  I've been working on closing other gaps.
>
>I'm really happy that someone has taken the time to work on this.
> Instead of derailing their attempt of extending CPIO to include
>xattrs, I'd appreciate your making constructive suggestions.
>
>Mimi

Do you have a description of the gaps you have identified?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2018-02-18 00:49:15

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH v3 01/15] Documentation: add newcx initramfs format description

On Sat, 2018-02-17 at 16:26 -0800, [email protected] wrote:

> Do you have a description of the gaps you have identified?

Probably the 2016 Linux Security Summit (LSS) integrity status update
has the best list.

http://events17.linuxfoundation.org/sites/events/files/slides/LSS2016-
LinuxIntegritySubsystemStatus.pdf

Mimi


2018-02-20 19:01:16

by Stephen Smalley

[permalink] [raw]
Subject: Re: [PATCH v3 14/15] selinux: allow setxattr on rootfs so initramfs code can set them

On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
> From: Victor Kamensky <[email protected]>
>
> initramfs code supporting extended cpio format have ability to
> fill extended attributes from cpio archive, but if SELinux enabled
> and security server is not initialized yet, selinux callback would
> refuse setxattr made by initramfs code.
>
> Solution enable SBLABEL_MNT on rootfs even if secrurity server is
> not initialized yet.

What if we were to instead skip the SBLABEL_MNT check in
selinux_inode_setxattr() if !ss_initialized? Not dependent on
filesystem type.

>
> Signed-off-by: Victor Kamensky <[email protected]>
> ---
> security/selinux/hooks.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8644d864e3c1..f3fe65589f02 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -706,6 +706,18 @@ static int selinux_set_mnt_opts(struct
> super_block *sb,
>
> if (!ss_initialized) {
> if (!num_opts) {
> + /*
> + * Special handling for rootfs. Is genfs but
> supports
> + * setting SELinux context on in-core
> inodes.
> + *
> + * Chicken and egg problem: policy may
> reside in rootfs
> + * but for initramfs code to fill in
> attributes, it
> + * needs selinux to allow that.
> + */
> + if (!strncmp(sb->s_type->name, "rootfs",
> + sizeof("rootfs")))
> + sbsec->flags |= SBLABEL_MNT;
> +
> /* Defer initialization until
> selinux_complete_init,
> after the initial policy is loaded and
> the security
> server is ready to handle calls. */

2018-02-20 19:06:45

by Stephen Smalley

[permalink] [raw]
Subject: Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete

On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
> From: Victor Kamensky <[email protected]>
>
> With initramfs cpio format that supports extended attributes
> we need to skip sid population on sys_lsetxattr call from
> initramfs for rootfs if security server is not initialized yet.
>
> Otherwise callback in selinux_inode_post_setxattr will try to
> translate give security.selinux label into sid context and since
> security server is not available yet inode will receive default
> sid (typically kernel_t). Note that in the same time proper
> label will be stored in inode xattrs. Later, since inode sid
> would be already populated system will never look back at
> actual xattrs. But if we skip sid population for rootfs and
> we have policy that direct use of xattrs for rootfs, proper
> sid will be filled in from extended attributes one node is
> accessed and server is initialized.
>
> Note new DELAYAFTERINIT_MNT super block flag is introduced
> to only mark rootfs for such behavior. For other types of
> tmpfs original logic is still used.

(cc selinux maintainers)

Wondering if we shouldn't just do this always, for all filesystem
types. Also, I think this should likely also be done in
selinux_inode_setsecurity() for consistency.

>
> Signed-off-by: Victor Kamensky <[email protected]>
> ---
> security/selinux/hooks.c | 9 ++++++++-
> security/selinux/include/security.h | 1 +
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f3fe65589f02..bb25268f734e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -716,7 +716,7 @@ static int selinux_set_mnt_opts(struct
> super_block *sb,
> */
> if (!strncmp(sb->s_type->name, "rootfs",
> sizeof("rootfs")))
> - sbsec->flags |= SBLABEL_MNT;
> + sbsec->flags |=
> SBLABEL_MNT|DELAYAFTERINIT_MNT;
>
> /* Defer initialization until
> selinux_complete_init,
> after the initial policy is loaded and
> the security
> @@ -3253,6 +3253,7 @@ static void selinux_inode_post_setxattr(struct
> dentry *dentry, const char *name,
> {
> struct inode *inode = d_backing_inode(dentry);
> struct inode_security_struct *isec;
> + struct superblock_security_struct *sbsec;
> u32 newsid;
> int rc;
>
> @@ -3261,6 +3262,12 @@ static void selinux_inode_post_setxattr(struct
> dentry *dentry, const char *name,
> return;
> }
>
> + if (!ss_initialized) {
> + sbsec = inode->i_sb->s_security;
> + if (sbsec->flags & DELAYAFTERINIT_MNT)
> + return;
> + }
> +
> rc = security_context_to_sid_force(value, size, &newsid);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to map context to
> SID"
> diff --git a/security/selinux/include/security.h
> b/security/selinux/include/security.h
> index 02f0412d42f2..585acfd6cbcf 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -52,6 +52,7 @@
> #define ROOTCONTEXT_MNT 0x04
> #define DEFCONTEXT_MNT 0x08
> #define SBLABEL_MNT 0x10
> +#define DELAYAFTERINIT_MNT 0x20
> /* Non-mount related flags */
> #define SE_SBINITIALIZED 0x0100
> #define SE_SBPROC 0x0200

2018-03-07 16:55:29

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete

On 02/20/2018 12:56 PM, Stephen Smalley wrote:
> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>> From: Victor Kamensky <[email protected]>
>>
>> With initramfs cpio format that supports extended attributes
>> we need to skip sid population on sys_lsetxattr call from
>> initramfs for rootfs if security server is not initialized yet.
>>
>> Otherwise callback in selinux_inode_post_setxattr will try to
>> translate give security.selinux label into sid context and since
>> security server is not available yet inode will receive default
>> sid (typically kernel_t). Note that in the same time proper
>> label will be stored in inode xattrs. Later, since inode sid
>> would be already populated system will never look back at
>> actual xattrs. But if we skip sid population for rootfs and
>> we have policy that direct use of xattrs for rootfs, proper
>> sid will be filled in from extended attributes one node is
>> accessed and server is initialized.
>>
>> Note new DELAYAFTERINIT_MNT super block flag is introduced
>> to only mark rootfs for such behavior. For other types of
>> tmpfs original logic is still used.
>
> (cc selinux maintainers)
>
> Wondering if we shouldn't just do this always, for all filesystem
> types. Also, I think this should likely also be done in
> selinux_inode_setsecurity() for consistency.

I don't understand what selinux thinks it's doing here.

Initramfs is special because it's populated early, ideally early enough drivers
can load their firmware out of it. This is guaranteed to be before any processes
have launched, before any other filesystems have been mounted. I'm surprised
selinux is trying to do anything this early because A) what is there for it to
do, B) where did it get a ruleset?

This isn't really a mount flag, this is a "the selinux subsystem isn't
functionally initialized yet" flag. We haven't launched init. In a modular
system the module probably isn't loaded. There are no processes, and the only
files anywhere are the ones we're in the process of extracting. What's there
fore selinux to do?

When a filesystem is mounted, none of these cached selinux "we already looked at
the xattrs" inode fields are populated yet, correct? It can figure that out when
something accesses the file and do it then, so the point is _not_ doing this now
and thus not cacheing the wrong info. That's what the mount flag is doing,
telling selinux "not yet". So why does selinux not already _know_ "not yet"?

Why doesn't load_policy flush the cache of the old default contexts? What
happens if you mount an ext2 root and then init reads a dozen files before it
gets to the load_policy? Do those doesn't files have bad default contexts
forever now?

Where does the selinux ruleset come from during the cpio extract? Was it
hardwired into the driver? It certainly didn't come out of a file, and it wasn't
a process that loaded it. Why is selinux trying to evaluate and cache the
security context of files before it has any rules? (It has xattr annotations,
but they have no _meaning_ without rules...?

Confused,

Rob

2018-03-07 17:37:01

by Victor Kamensky

[permalink] [raw]
Subject: Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete



On Wed, 7 Mar 2018, Rob Landley wrote:

> On 02/20/2018 12:56 PM, Stephen Smalley wrote:
>> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>>> From: Victor Kamensky <[email protected]>
>>>
>>> With initramfs cpio format that supports extended attributes
>>> we need to skip sid population on sys_lsetxattr call from
>>> initramfs for rootfs if security server is not initialized yet.
>>>
>>> Otherwise callback in selinux_inode_post_setxattr will try to
>>> translate give security.selinux label into sid context and since
>>> security server is not available yet inode will receive default
>>> sid (typically kernel_t). Note that in the same time proper
>>> label will be stored in inode xattrs. Later, since inode sid
>>> would be already populated system will never look back at
>>> actual xattrs. But if we skip sid population for rootfs and
>>> we have policy that direct use of xattrs for rootfs, proper
>>> sid will be filled in from extended attributes one node is
>>> accessed and server is initialized.
>>>
>>> Note new DELAYAFTERINIT_MNT super block flag is introduced
>>> to only mark rootfs for such behavior. For other types of
>>> tmpfs original logic is still used.
>>
>> (cc selinux maintainers)
>>
>> Wondering if we shouldn't just do this always, for all filesystem
>> types. Also, I think this should likely also be done in
>> selinux_inode_setsecurity() for consistency.

Sorry, I did not have time to try out Stephen's suggestion,
especially given that core initramfs xattrs acceptance and dicussion
looks a bit stalled, and for my use case it is dependency before
SELinux changes.

I will look for both suggestion this week. Hope to see initramfs
xattrs patch series review going again.

> I don't understand what selinux thinks it's doing here.
>
> Initramfs is special because it's populated early, ideally early enough drivers
> can load their firmware out of it. This is guaranteed to be before any processes
> have launched, before any other filesystems have been mounted. I'm surprised
> selinux is trying to do anything this early because A) what is there for it to
> do, B) where did it get a ruleset?
>
> This isn't really a mount flag, this is a "the selinux subsystem isn't
> functionally initialized yet" flag. We haven't launched init. In a modular
> system the module probably isn't loaded. There are no processes, and the only
> files anywhere are the ones we're in the process of extracting. What's there
> fore selinux to do?
>
> When a filesystem is mounted, none of these cached selinux "we already looked at
> the xattrs" inode fields are populated yet, correct? It can figure that out when
> something accesses the file and do it then, so the point is _not_ doing this now
> and thus not cacheing the wrong info. That's what the mount flag is doing,
> telling selinux "not yet". So why does selinux not already _know_ "not yet"?
>
> Why doesn't load_policy flush the cache of the old default contexts? What
> happens if you mount an ext2 root and then init reads a dozen files before it
> gets to the load_policy?

I need to check whether security context caching happens on all
file operations, or when setxattr is executed. If latter,
setxattr operation before policy load may not be very common use case.

Also note there is a second SELinux related patch and
corresponding Stephen's comment: if SELinux is enabled
in kernel, but policy is not loaded yet, setxattr for security.selinux
extended attribute will go for check to SELinux LSM callback, it will
be denied. My other patch was relaxing above for "rootfs" only,
i.e covering initramfs xattrs case. Stephen's point was that
maybe it needs to be relaxed for
all cases if policy not loaded yet. I need some time to look
at the code and think about what can go wrong, if rule relaxed
for all cases.

> Do those doesn't files have bad default contexts forever now?
>
> Where does the selinux ruleset come from during the cpio extract?

Yes, in our use case SELinux policy file
(/etc/selinux/*/policy/policy.*) comes from cpio initramfs itself.
So it is chicken and egg problem.

> Was it
> hardwired into the driver? It certainly didn't come out of a file, and it wasn't
> a process that loaded it. Why is selinux trying to evaluate and cache the
> security context of files before it has any rules?

Note for ext2 case there is no setxattr first as we have in initramfs
xattrs case, so extended attributes values are read from backing
persitent storage as they were put there before, and there would not
be discrepency what is cached in security context inode data scructure
and real "security.selinux" extended attribute value in file system.

Thanks,
Victor

> (It has xattr annotations,
> but they have no _meaning_ without rules...?
>
> Confused,
>
> Rob
>

2018-03-11 03:11:18

by Victor Kamensky

[permalink] [raw]
Subject: Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete



On Tue, 20 Feb 2018, Stephen Smalley wrote:

> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>> From: Victor Kamensky <[email protected]>
>>
>> With initramfs cpio format that supports extended attributes
>> we need to skip sid population on sys_lsetxattr call from
>> initramfs for rootfs if security server is not initialized yet.
>>
>> Otherwise callback in selinux_inode_post_setxattr will try to
>> translate give security.selinux label into sid context and since
>> security server is not available yet inode will receive default
>> sid (typically kernel_t). Note that in the same time proper
>> label will be stored in inode xattrs. Later, since inode sid
>> would be already populated system will never look back at
>> actual xattrs. But if we skip sid population for rootfs and
>> we have policy that direct use of xattrs for rootfs, proper
>> sid will be filled in from extended attributes one node is
>> accessed and server is initialized.
>>
>> Note new DELAYAFTERINIT_MNT super block flag is introduced
>> to only mark rootfs for such behavior. For other types of
>> tmpfs original logic is still used.
>
> (cc selinux maintainers)
>
> Wondering if we shouldn't just do this always, for all filesystem
> types.

Ok, I think it makes sense. The one that do not support xattrs
will not reach selinux_inode_post_setxattr anyway. And try
to cache sid while !ss_initialized is not good idea for any
filesystem types.

> Also, I think this should likely also be done in
> selinux_inode_setsecurity() for consistency.

I am not sure that I follow selinux_inode_setsecurity suggestion.
selinux_inode_setsecurity is about permission check. And
selinux_inode_post_setxattr deals with processing and setting
side effects if xattr was "security.selinux", it does not
matter what happens in selinux_inode_setsecurity if it
returns access_ok, LSM will still call selinux_inode_post_setxattr
and we would need to check and not produce any sid caching
side effects if !ss_initialized.

Sitll keeping logic in selinux_inode_post_setxattr, checked
that the following with much simple code works too:

>From bfc54e4805f3059671417ff2cda1266bc68e18f9 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <[email protected]>
Date: Fri, 9 Mar 2018 23:06:08 -0800
Subject: [PATCH 2/2] selinux: delay sid population in setxattr till policy
loaded

With initramfs cpio format that supports extended attributes
we need to skip sid population on sys_lsetxattr call from
initramfs for rootfs if security server is not initialized yet.

Otherwise callback in selinux_inode_post_setxattr will try to
translate give security.selinux label into sid context and since
security server is not available yet inode will receive default
sid (typically kernel_t). Note that in the same time proper
label will be stored in inode xattrs. Later, since inode sid
would be already populated system will never look back at
actual xattrs. But if we skip sid population for rootfs and
we have policy that direct use of xattrs for rootfs, proper
sid will be filled in from extended attributes one node is
accessed and server is initialized.

Signed-off-by: Victor Kamensky <[email protected]>
---
security/selinux/hooks.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 31303ed..4c13759 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3197,6 +3197,10 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
return;
}

+ if (!ss_initialized) {
+ return;
+ }
+
rc = security_context_to_sid_force(value, size, &newsid);
if (rc) {
printk(KERN_ERR "SELinux: unable to map context to SID"
--
2.7.4

Thanks,
Victor

>>
>> Signed-off-by: Victor Kamensky <[email protected]>
>> ---
>> security/selinux/hooks.c | 9 ++++++++-
>> security/selinux/include/security.h | 1 +
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index f3fe65589f02..bb25268f734e 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -716,7 +716,7 @@ static int selinux_set_mnt_opts(struct
>> super_block *sb,
>> */
>> if (!strncmp(sb->s_type->name, "rootfs",
>> sizeof("rootfs")))
>> - sbsec->flags |= SBLABEL_MNT;
>> + sbsec->flags |=
>> SBLABEL_MNT|DELAYAFTERINIT_MNT;
>>
>> /* Defer initialization until
>> selinux_complete_init,
>> after the initial policy is loaded and
>> the security
>> @@ -3253,6 +3253,7 @@ static void selinux_inode_post_setxattr(struct
>> dentry *dentry, const char *name,
>> {
>> struct inode *inode = d_backing_inode(dentry);
>> struct inode_security_struct *isec;
>> + struct superblock_security_struct *sbsec;
>> u32 newsid;
>> int rc;
>>
>> @@ -3261,6 +3262,12 @@ static void selinux_inode_post_setxattr(struct
>> dentry *dentry, const char *name,
>> return;
>> }
>>
>> + if (!ss_initialized) {
>> + sbsec = inode->i_sb->s_security;
>> + if (sbsec->flags & DELAYAFTERINIT_MNT)
>> + return;
>> + }
>> +
>> rc = security_context_to_sid_force(value, size, &newsid);
>> if (rc) {
>> printk(KERN_ERR "SELinux: unable to map context to
>> SID"
>> diff --git a/security/selinux/include/security.h
>> b/security/selinux/include/security.h
>> index 02f0412d42f2..585acfd6cbcf 100644
>> --- a/security/selinux/include/security.h
>> +++ b/security/selinux/include/security.h
>> @@ -52,6 +52,7 @@
>> #define ROOTCONTEXT_MNT 0x04
>> #define DEFCONTEXT_MNT 0x08
>> #define SBLABEL_MNT 0x10
>> +#define DELAYAFTERINIT_MNT 0x20
>> /* Non-mount related flags */
>> #define SE_SBINITIALIZED 0x0100
>> #define SE_SBPROC 0x0200
>

2018-03-11 03:11:18

by Victor Kamensky

[permalink] [raw]
Subject: Re: [PATCH v3 14/15] selinux: allow setxattr on rootfs so initramfs code can set them



On Tue, 20 Feb 2018, Stephen Smalley wrote:

> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>> From: Victor Kamensky <[email protected]>
>>
>> initramfs code supporting extended cpio format have ability to
>> fill extended attributes from cpio archive, but if SELinux enabled
>> and security server is not initialized yet, selinux callback would
>> refuse setxattr made by initramfs code.
>>
>> Solution enable SBLABEL_MNT on rootfs even if secrurity server is
>> not initialized yet.
>
> What if we were to instead skip the SBLABEL_MNT check in
> selinux_inode_setxattr() if !ss_initialized? Not dependent on
> filesystem type.

Stephen, thank you for looking into this. Sorry, for dealyed reponse -
I needed to find time to require context about these changes.

As you suggested I've tried this and it works:

>From 6bf35bd055fdb12e94f3d5188eccfdbaa30dbcf4 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <[email protected]>
Date: Fri, 9 Mar 2018 23:01:20 -0800
Subject: [PATCH 1/2] selinux: allow setxattr on file systems if policy is not
loaded

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code because file system is not
yet marked as one that support labeling (SBLABEL_MNT flag).

Solution do not refuse setxattr even if SBLABEL_MNT is not set
for file systems when policy is not loaded yet.

Signed-off-by: Victor Kamensky <[email protected]>
---
security/selinux/hooks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd68..31303ed 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3120,7 +3120,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
return selinux_inode_setotherxattr(dentry, name);

sbsec = inode->i_sb->s_security;
- if (!(sbsec->flags & SBLABEL_MNT))
+ if (!(sbsec->flags & SBLABEL_MNT) && ss_initialized)
return -EOPNOTSUPP;

if (!inode_owner_or_capable(inode))
--
2.7.4

But with this change it would mean for that filesystem types, that
never are supposed to get SBLABEL_MNT flag, code may go through
if !ss_initialized. I have hard time evaluating impication of this,
but it is not existing case or not a big deal.

Generally I agree with your concern that the issue is not "rootfs"
specific. Other thought that it could be solved with use of
selinux_is_sblabel_mnt instead of "rootfs" specific check inside
of selinux_set_mnt_opts, in addition to similar code
in sb_finish_set_opts function. I.e something like this:

>From a94548b5ecde43ccc9c2b02b29becc086b4343a3 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <[email protected]>
Date: Fri, 9 Mar 2018 23:01:20 -0800
Subject: [PATCH 1/2] selinux: allow setxattr on rootfs so initramfs code can
set them

initramfs code supporting extended cpio format have ability to
fill extended attributes from cpio archive, but if SELinux enabled
and security server is not initialized yet, selinux callback would
refuse setxattr made by initramfs code.

Solution enable set SBLABEL_MNT on file systems for which we can
figure out that they support securit labels even if secrurity
server is not initialized yet.

Signed-off-by: Victor Kamensky <[email protected]>
---
security/selinux/hooks.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 819fd68..326aca9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -701,6 +701,18 @@ static int selinux_set_mnt_opts(struct super_block *sb,

if (!ss_initialized) {
if (!num_opts) {
+ /*
+ * For some of file systems we can mark them as
+ * supporting security labels even before policy
+ * loaded. It may be used by code that may want
+ * to do setxatts before polict load.
+ *
+ * Note after policy loaded this check and marking
+ * happens again.
+ */
+ if (selinux_is_sblabel_mnt(sb))
+ sbsec->flags |= SBLABEL_MNT;
+
/* Defer initialization until selinux_complete_init,
after the initial policy is loaded and the security
server is ready to handle calls. */
--
2.7.4

Thanks,
Victor

>>
>> Signed-off-by: Victor Kamensky <[email protected]>
>> ---
>> security/selinux/hooks.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 8644d864e3c1..f3fe65589f02 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -706,6 +706,18 @@ static int selinux_set_mnt_opts(struct
>> super_block *sb,
>>
>> if (!ss_initialized) {
>> if (!num_opts) {
>> + /*
>> + * Special handling for rootfs. Is genfs but
>> supports
>> + * setting SELinux context on in-core
>> inodes.
>> + *
>> + * Chicken and egg problem: policy may
>> reside in rootfs
>> + * but for initramfs code to fill in
>> attributes, it
>> + * needs selinux to allow that.
>> + */
>> + if (!strncmp(sb->s_type->name, "rootfs",
>> + sizeof("rootfs")))
>> + sbsec->flags |= SBLABEL_MNT;
>> +
>> /* Defer initialization until
>> selinux_complete_init,
>> after the initial policy is loaded and
>> the security
>> server is ready to handle calls. */
>

2018-03-20 16:37:35

by Stephen Smalley

[permalink] [raw]
Subject: Re: [Non-DoD Source] Re: [PATCH v3 14/15] selinux: allow setxattr on rootfs so initramfs code can set them

On 03/10/2018 10:07 PM, Victor Kamensky wrote:
>
>
> On Tue, 20 Feb 2018, Stephen Smalley wrote:
>
>> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>>> From: Victor Kamensky <[email protected]>
>>>
>>> initramfs code supporting extended cpio format have ability to
>>> fill extended attributes from cpio archive, but if SELinux enabled
>>> and security server is not initialized yet, selinux callback would
>>> refuse setxattr made by initramfs code.
>>>
>>> Solution enable SBLABEL_MNT on rootfs even if secrurity server is
>>> not initialized yet.
>>
>> What if we were to instead skip the SBLABEL_MNT check in
>> selinux_inode_setxattr() if !ss_initialized?  Not dependent on
>> filesystem type.
>
> Stephen, thank you for looking into this. Sorry, for dealyed reponse -
> I needed to find time to require context about these changes.
>
> As you suggested I've tried this and it works:
>
>> From 6bf35bd055fdb12e94f3d5188eccfdbaa30dbcf4 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <[email protected]>
> Date: Fri, 9 Mar 2018 23:01:20 -0800
> Subject: [PATCH 1/2] selinux: allow setxattr on file systems if policy is not
>  loaded
>
> initramfs code supporting extended cpio format have ability to
> fill extended attributes from cpio archive, but if SELinux enabled
> and security server is not initialized yet, selinux callback would
> refuse setxattr made by initramfs code because file system is not
> yet marked as one that support labeling (SBLABEL_MNT flag).
>
> Solution do not refuse setxattr even if SBLABEL_MNT is not set
> for file systems when policy is not loaded yet.
>
> Signed-off-by: Victor Kamensky <[email protected]>
> ---
>  security/selinux/hooks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 819fd68..31303ed 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3120,7 +3120,7 @@ static int selinux_inode_setxattr(struct dentry *dentry, const char *name,
>          return selinux_inode_setotherxattr(dentry, name);
>
>      sbsec = inode->i_sb->s_security;
> -    if (!(sbsec->flags & SBLABEL_MNT))
> +    if (!(sbsec->flags & SBLABEL_MNT) && ss_initialized)
>          return -EOPNOTSUPP;
>
>      if (!inode_owner_or_capable(inode))

I favor the first option.

2018-03-20 16:40:23

by Stephen Smalley

[permalink] [raw]
Subject: Re: [Non-DoD Source] Re: [PATCH v3 15/15] selinux: delay sid population for rootfs till init is complete

On 03/10/2018 10:08 PM, Victor Kamensky wrote:
>
>
> On Tue, 20 Feb 2018, Stephen Smalley wrote:
>
>> On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
>>> From: Victor Kamensky <[email protected]>
>>>
>>> With initramfs cpio format that supports extended attributes
>>> we need to skip sid population on sys_lsetxattr call from
>>> initramfs for rootfs if security server is not initialized yet.
>>>
>>> Otherwise callback in selinux_inode_post_setxattr will try to
>>> translate give security.selinux label into sid context and since
>>> security server is not available yet inode will receive default
>>> sid (typically kernel_t). Note that in the same time proper
>>> label will be stored in inode xattrs. Later, since inode sid
>>> would be already populated system will never look back at
>>> actual xattrs. But if we skip sid population for rootfs and
>>> we have policy that direct use of xattrs for rootfs, proper
>>> sid will be filled in from extended attributes one node is
>>> accessed and server is initialized.
>>>
>>> Note new DELAYAFTERINIT_MNT super block flag is introduced
>>> to only mark rootfs for such behavior. For other types of
>>> tmpfs original logic is still used.
>>
>> (cc selinux maintainers)
>>
>> Wondering if we shouldn't just do this always, for all filesystem
>> types.
>
> Ok, I think it makes sense. The one that do not support xattrs
> will not reach selinux_inode_post_setxattr anyway. And try
> to cache sid while !ss_initialized is not good idea for any
> filesystem types.
>
>> Also, I think this should likely also be done in
>> selinux_inode_setsecurity() for consistency.
>
> I am not sure that I follow selinux_inode_setsecurity suggestion.
> selinux_inode_setsecurity is about permission check. And
> selinux_inode_post_setxattr deals with processing and setting
> side effects if xattr was "security.selinux", it does not
> matter what happens in selinux_inode_setsecurity if it
> returns access_ok, LSM will still call selinux_inode_post_setxattr
> and we would need to check and not produce any sid caching
> side effects if !ss_initialized.

selinux_inode_setsecurity is the vfs fallback for setting security
attributes when the filesystem/inode does not support setxattr itself,
and is also used by kernfs.
So you need to update both selinux_inode_post_setxattr and selinux_inode_setsecurity
in the same way.

>
> Sitll keeping logic in selinux_inode_post_setxattr, checked
> that the following with much simple code works too:
>
>> From bfc54e4805f3059671417ff2cda1266bc68e18f9 Mon Sep 17 00:00:00 2001
> From: Victor Kamensky <[email protected]>
> Date: Fri, 9 Mar 2018 23:06:08 -0800
> Subject: [PATCH 2/2] selinux: delay sid population in setxattr till policy
>  loaded
>
> With initramfs cpio format that supports extended attributes
> we need to skip sid population on sys_lsetxattr call from
> initramfs for rootfs if security server is not initialized yet.
>
> Otherwise callback in selinux_inode_post_setxattr will try to
> translate give security.selinux label into sid context and since
> security server is not available yet inode will receive default
> sid (typically kernel_t). Note that in the same time proper
> label will be stored in inode xattrs. Later, since inode sid
> would be already populated system will never look back at
> actual xattrs. But if we skip sid population for rootfs and
> we have policy that direct use of xattrs for rootfs, proper
> sid will be filled in from extended attributes one node is
> accessed and server is initialized.
>
> Signed-off-by: Victor Kamensky <[email protected]>
> ---
>  security/selinux/hooks.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 31303ed..4c13759 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3197,6 +3197,10 @@ static void selinux_inode_post_setxattr(struct dentry *dentry, const char *name,
>          return;
>      }
>
> +        if (!ss_initialized) {
> +                return;
> +        }
> +
>      rc = security_context_to_sid_force(value, size, &newsid);
>      if (rc) {
>          printk(KERN_ERR "SELinux:  unable to map context to SID"