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/initramfs at vger.kernel.org/msg03971.html
Latest discussion I was able to find is from Dec 2015:
https://www.mail-archive.com/initramfs at vger.kernel.org/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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
Signed-off-by: Mimi Zohar <redacted>
Signed-off-by: Victor Kamensky <redacted>
---
Documentation/early-userspace/buffer-format.txt | 46 ++++++++++++++++++++++---
1 file changed, 41 insertions(+), 5 deletions(-)
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
init/initramfs.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
@@ -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;return0;
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
init/initramfs.c | 121 +++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 96 insertions(+), 25 deletions(-)
@@ -315,22 +364,34 @@ static int __init do_collect(void)staticint__initdo_format(void){-if(memcmp(collected,"070707",CPIO_MAGIC_SIZE)==0){+intheader_size=0;++cpio_format=CPIO_NO_MAGIC;++if(!memcmp(collected,"070707",CPIO_MAGIC_SIZE)){error("incorrect cpio method used: use -H newc option");return1;+}elseif(!memcmp(collected,"070701",CPIO_MAGIC_SIZE)){+cpio_format=CPIO_NEWC;+header_size=sizeof(structcpio_newc_header);+}elseif(!memcmp(collected,"070703",CPIO_MAGIC_SIZE)){+cpio_format=CPIO_NEWCX;+header_size=sizeof(structcpio_newcx_header);}-if(memcmp(collected,"070701",CPIO_MAGIC_SIZE)){++if(cpio_format==CPIO_NO_MAGIC){error("no cpio magic");return1;}-read_into(header_buf,sizeof(structcpio_newc_header),do_header);+read_into(header_buf,header_size,do_header);return0;}staticint__initdo_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);return0;}+staticint__initdo_xattrs(void)+{+/* Do nothing for now */+state=do_create;+return0;+}static__initdataintwfd;
@@ -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);}elseif(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);}}elseif(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;return0;
@@ -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;return0;
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
usr/gen_init_cpio.c | 69 +++++++++++++++++++++++++++++++++++++----------------
1 file changed, 48 insertions(+), 21 deletions(-)
@@ -450,7 +473,7 @@ static int cpio_mkfile_line(const char *line)staticvoidusage(constchar*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){-intopt=getopt(argc,argv,"t:h");+intopt=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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -29,7 +29,8 @@ ramfs-input := $(if $(filter-out "",$(CONFIG_INITRAMFS_SOURCE)), \$(shellecho$(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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -29,7 +29,8 @@ ramfs-input := $(if $(filter-out "",$(CONFIG_INITRAMFS_SOURCE)), \$(shellecho$(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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <redacted>
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 <redacted>
---
security/selinux/hooks.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -706,6 +706,18 @@ static int selinux_set_mnt_opts(struct super_block *sb,if(!ss_initialized){if(!num_opts){+/*+*Specialhandlingforrootfs.Isgenfsbutsupports+*settingSELinuxcontextonin-coreinodes.+*+*Chickenandeggproblem:policymayresideinrootfs+*butforinitramfscodetofillinattributes,it+*needsselinuxtoallowthat.+*/+if(!strncmp(sb->s_type->name,"rootfs",+sizeof("rootfs")))+sbsec->flags|=SBLABEL_MNT;+/* Defer initialization until selinux_complete_init,aftertheinitialpolicyisloadedandthesecurityserverisreadytohandlecalls.*/
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <redacted>
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 <redacted>
---
security/selinux/hooks.c | 9 ++++++++-
security/selinux/include/security.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <redacted>
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 <redacted>
---
security/selinux/hooks.c | 9 ++++++++-
security/selinux/include/security.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <redacted>
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 <redacted>
---
security/selinux/hooks.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
@@ -706,6 +706,18 @@ static int selinux_set_mnt_opts(struct super_block *sb,if(!ss_initialized){if(!num_opts){+/*+*Specialhandlingforrootfs.Isgenfsbutsupports+*settingSELinuxcontextonin-coreinodes.+*+*Chickenandeggproblem:policymayresideinrootfs+*butforinitramfscodetofillinattributes,it+*needsselinuxtoallowthat.+*/+if(!strncmp(sb->s_type->name,"rootfs",+sizeof("rootfs")))+sbsec->flags|=SBLABEL_MNT;+/* Defer initialization until selinux_complete_init,aftertheinitialpolicyisloadedandthesecurityserverisreadytohandlecalls.*/
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
usr/gen_init_cpio.c | 144 +++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 121 insertions(+), 23 deletions(-)
@@ -144,6 +136,96 @@ static void cpio_trailer(void)}}+structxattr_hdr{+charc_size[8];/* total size including c_size field */+charc_data[];+};+staticunsignedintxattr_buflen;+staticcharxattr_buf[4096];++staticvoidpush_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();+}++staticintconvert_hex_string(constchar*hex_str,char*out,size_tout_size)+{+charbuf[3];+size_tstr_len=strlen(hex_str);++if(str_len%2!=0||str_len/2>out_size)+return0;++buf[2]='\0';+while(*hex_str!='\0'){+buf[0]=*hex_str++;+buf[1]=*hex_str++;+*out++=(char)strtol(buf,NULL,16);+}++returnstr_len/2;+}++staticintcollect_xattr(constchar*line)+{+constchar*name,*value;+size_tname_len,value_len;+char*buf=xattr_buf+xattr_buflen;+structxattr_hdr*hdr=(structxattr_hdr*)buf;+char*bufend=xattr_buf+sizeof(xattr_buf);+char*value_buf;+size_txattr_entry_size;+charsize_str[sizeof(hdr->c_size)+1];++if(!newcx)+return0;++name=line;+value=strchr(line,'=');+if(!value){+fprintf(stderr,"Unrecognized xattr format '%s'",line);+return-1;+}+name_len=value-name;+value++;++/*+*Fornowwesupportonlyhexencodedvalues.+*Stringorbase64canbeaddedlater.+*/+if(strncmp(value,"0x",2)){+fprintf(stderr,+"Only hex encoded xattr value is supported '%s'",+value);+return-1;+}++value+=2;+value_buf=buf+sizeof(structxattr_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(structxattr_hdr)+name_len+1+value_len;++sprintf(size_str,"%08X",(unsignedint)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;++return0;+}+staticintcpio_mkslink(constchar*name,constchar*target,unsignedintmode,uid_tuid,gid_tgid){
@@ -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();return0;}
@@ -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();return0;}
@@ -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"
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
usr/gen_init_cpio.c | 186 ++++++++++++++++++++++++++--------------------------
1 file changed, 92 insertions(+), 94 deletions(-)
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Mimi Zohar <redacted>
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 <redacted>
Signed-off-by: Victor Kamensky <redacted>
Signed-off-by: Taras Kondratiuk <redacted>
---
init/initramfs.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 52 insertions(+), 5 deletions(-)
@@ -306,6 +306,7 @@ static int __init do_xattrs(void);staticint__initdo_create(void);staticint__initdo_copy(void);staticint__initdo_symlink(void);+staticint__initdo_setxattrs(void);staticint__initdo_reset(void);typedefint(*fsm_state_t)(void);
@@ -468,7 +469,7 @@ static int __init do_name(void)staticint__initdo_xattrs(void){-/* Do nothing for now */+memcpy_optional(xattr_buf,collected,xattr_len);state=do_create;return0;}
@@ -477,8 +478,7 @@ static __initdata int wfd;staticint__initdo_create(void){-state=do_skip;-next_state=do_reset;+state=do_setxattrs;clean_path(name_buf,mode);if(S_ISREG(mode)){intml=maybe_link(name_buf);
@@ -511,8 +511,11 @@ static int __init do_create(void)do_utime(name_buf,&mtime);}}elseif(S_ISLNK(mode)){-if(body_len>PATH_MAX)+if(body_len>PATH_MAX){+state=do_skip;+next_state=do_reset;return0;+}read_into(symlink_buf,body_len,do_symlink);}return0;
@@ -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;return0;}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;+return0;+}++structxattr_hdr{+charc_size[8];/* total size including c_size field */+charc_data[];/* <name>\0<value> */+};++staticint__initdo_setxattrs(void)+{+char*buf=xattr_buf;+char*bufend=buf+xattr_len;+structxattr_hdr*hdr;+charstr[sizeof(hdr->c_size)+1];+state=do_skip;next_state=do_reset;+if(!xattr_len)+return0;++str[sizeof(hdr->c_size)]=0;++while(buf<bufend){+char*xattr_name,*xattr_value;+unsignedlongxattr_entry_size,xattr_value_size;+intret;++hdr=(structxattr_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);+}return0;}
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
init/initramfs.c | 122 +++++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 92 insertions(+), 30 deletions(-)
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
init/initramfs.c | 52 ++++++++++++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 22 deletions(-)
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
init/initramfs.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
--
2.10.3.dirty
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <redacted>
---
init/initramfs.c | 73 +++++++++++++++++++++++++-------------------------------
1 file changed, 32 insertions(+), 41 deletions(-)
@@ -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)return0;if(S_ISLNK(mode)){
@@ -271,12 +272,12 @@ static int __init do_header(void)return0;collect=collected=symlink_buf;remains=N_ALIGN(name_len)+body_len;-next_state=GotSymlink;-state=Collect;+next_state=do_symlink;+state=do_collect;return0;}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);return0;}
@@ -327,8 +328,8 @@ static __initdata int wfd;staticint__initdo_name(void){-state=SkipIt;-next_state=Reset;+state=do_skip;+next_state=do_reset;if(strcmp(collected,"TRAILER!!!")==0){free_hash();return0;
@@ -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;}}}elseif(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;return0;}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;return0;}-static__initdataint(*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,-};-staticlong__initwrite_buffer(char*buf,unsignedlonglen){byte_count=len;victim=buf;-while(!actions[state]())-;+do+pr_debug("state: %pf\n",state);+while(!state());returnlen-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;}elseif(c==0){buf+=written;len-=written;-state=Reset;+state=do_reset;}elseerror("junk in compressed archive");}
@@ -497,7 +488,7 @@ static char * __init unpack_to_rootfs(char *buf, unsigned long len)}}elseerror("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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Rob Landley <hidden> Date: 2018-02-16 21:25:16
On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
On 02/16/18 12:33, Taras Kondratiuk wrote:
quoted
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.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <hidden> Date: 2018-02-16 21:57:12
On Fri, 16 Feb 2018, Rob Landley wrote:
On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
quoted
On 02/16/18 12:33, Taras Kondratiuk wrote:
quoted
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?
quoted
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.
quoted
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?)
quoted
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.
quoted
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)
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.
quoted
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2018-02-16 22:03:56
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On February 16, 2018 1:47:35 PM PST, Victor Kamensky [off-list ref] wrote:
On Fri, 16 Feb 2018, Rob Landley wrote:
quoted
On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
quoted
On 02/16/18 12:33, Taras Kondratiuk wrote:
quoted
Many of the Linux security/integrity features are dependent on file
metadata, stored as extended attributes (xattrs), for making
decisions.
quoted
quoted
quoted
These features need to be initialized during initcall and enabled
as
quoted
quoted
quoted
early as possible for complete security coverage.
Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
does not
quoted
quoted
quoted
support including them into the archive.
This patch describes "extended" newc format (newcx) that is based
on
quoted
quoted
quoted
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,
quoted
quoted
you shouldn't replicate the mistakes of the current format.
Specifically:
quoted
So rather than make minimal changes to the existing format and
continue to
quoted
support the existing format (sharing as much code as possible), you
recommend
quoted
gratuitous aesthetic changes?
quoted
1. The use of ASCII-encoded fixed-length numbers is an idiotic
legacy
quoted
quoted
from an era before there were any portable way of dealing with
numbers
quoted
quoted
with prespecified endianness.
It lets encoders and decoders easily share code with the existing
cpio format,
quoted
which we still intend to be able to read and write.
quoted
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
quoted
different from the previous iteration?)
quoted
The cpio header isn't fixed size, so that argument goes away, in
fact
quoted
quoted
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
quoted
quoted
is padding before the data, and the size of that padding is only
defined
quoted
quoted
by alignment.
Again, these are minimal changes to the existing cpio format. You're
complaining
quoted
about _cpio_, and that the new stuff isn't _different_ enough from
it.
quoted
quoted
3. Inband encoding of EOF: if you actually have a filename
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.
quoted
There are links in
Documentation/filesystems/ramfs-rootfs-initramfs.txt to the
quoted
mailing list entries about it.
quoted
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
quoted
been distributed via tarball back since 0.0.1?)
You're suggesting having a whole second codepath that shares no code
with the
quoted
existing cpio extractor. Are you suggesting abandoning support for
the existing
quoted
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On February 16, 2018 1:47:35 PM PST, Victor Kamensky [off-list ref] wrote:
quoted
On Fri, 16 Feb 2018, Rob Landley wrote:
quoted
On 02/16/2018 02:59 PM, H. Peter Anvin wrote:
quoted
On 02/16/18 12:33, Taras Kondratiuk wrote:
quoted
Many of the Linux security/integrity features are dependent on file
metadata, stored as extended attributes (xattrs), for making
decisions.
quoted
quoted
quoted
These features need to be initialized during initcall and enabled
as
quoted
quoted
quoted
early as possible for complete security coverage.
Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
does not
quoted
quoted
quoted
support including them into the archive.
This patch describes "extended" newc format (newcx) that is based
on
quoted
quoted
quoted
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,
quoted
quoted
you shouldn't replicate the mistakes of the current format.
Specifically:
quoted
So rather than make minimal changes to the existing format and
continue to
quoted
support the existing format (sharing as much code as possible), you
recommend
quoted
gratuitous aesthetic changes?
quoted
1. The use of ASCII-encoded fixed-length numbers is an idiotic
legacy
quoted
quoted
from an era before there were any portable way of dealing with
numbers
quoted
quoted
with prespecified endianness.
It lets encoders and decoders easily share code with the existing
cpio format,
quoted
which we still intend to be able to read and write.
quoted
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
quoted
different from the previous iteration?)
quoted
The cpio header isn't fixed size, so that argument goes away, in
fact
quoted
quoted
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
quoted
quoted
is padding before the data, and the size of that padding is only
defined
quoted
quoted
by alignment.
Again, these are minimal changes to the existing cpio format. You're
complaining
quoted
about _cpio_, and that the new stuff isn't _different_ enough from
it.
quoted
quoted
3. Inband encoding of EOF: if you actually have a filename
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.
quoted
There are links in
Documentation/filesystems/ramfs-rootfs-initramfs.txt to the
quoted
mailing list entries about it.
quoted
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
quoted
been distributed via tarball back since 0.0.1?)
You're suggesting having a whole second codepath that shares no code
with the
quoted
existing cpio extractor. Are you suggesting abandoning support for
the existing
quoted
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html
From: Rob Landley <hidden> Date: 2018-02-17 17:33:00
On 02/16/2018 06:00 PM, hpa at zytor.com 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:
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2018-02-16 at 12:59 -0800, H. Peter Anvin wrote:
On 02/16/18 12:33, Taras Kondratiuk wrote:
quoted
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On February 17, 2018 4:15:12 PM PST, Mimi Zohar [off-list ref] wrote:
On Fri, 2018-02-16 at 12:59 -0800, H. Peter Anvin wrote:
quoted
On 02/16/18 12:33, Taras Kondratiuk wrote:
quoted
Many of the Linux security/integrity features are dependent on file
metadata, stored as extended attributes (xattrs), for making
decisions.
quoted
quoted
These features need to be initialized during initcall and enabled
as
quoted
quoted
early as possible for complete security coverage.
Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
does not
quoted
quoted
support including them into the archive.
This patch describes "extended" newc format (newcx) that is based
on
quoted
quoted
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:
quoted
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
quoted
with prespecified endianness. If you are going to use ASCII, make
them
quoted
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
quoted
is padding before the data, and the size of that padding is only
defined
quoted
by alignment.
3. Inband encoding of EOF: if you actually have a filename
"TRAILER!!!"
quoted
you have problems.
But first, before you define a whole new format for which no tools
exist
quoted
(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
quoted
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On February 17, 2018 4:15:12 PM PST, Mimi Zohar [off-list ref] wrote:
On Fri, 2018-02-16 at 12:59 -0800, H. Peter Anvin wrote:
quoted
On 02/16/18 12:33, Taras Kondratiuk wrote:
quoted
Many of the Linux security/integrity features are dependent on file
metadata, stored as extended attributes (xattrs), for making
decisions.
quoted
quoted
These features need to be initialized during initcall and enabled
as
quoted
quoted
early as possible for complete security coverage.
Initramfs (tmpfs) supports xattrs, but newc CPIO archive format
does not
quoted
quoted
support including them into the archive.
This patch describes "extended" newc format (newcx) that is based
on
quoted
quoted
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:
quoted
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
quoted
with prespecified endianness. If you are going to use ASCII, make
them
quoted
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
quoted
is padding before the data, and the size of that padding is only
defined
quoted
by alignment.
3. Inband encoding of EOF: if you actually have a filename
"TRAILER!!!"
quoted
you have problems.
But first, before you define a whole new format for which no tools
exist
quoted
(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
quoted
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stephen Smalley <hidden> Date: 2018-02-20 19:00:11
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
From: Victor Kamensky <redacted>
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.
@@ -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. */
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stephen Smalley <hidden> Date: 2018-02-20 19:05:47
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
From: Victor Kamensky <redacted>
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.
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Rob Landley <hidden> Date: 2018-03-07 16:53:33
On 02/20/2018 12:56 PM, Stephen Smalley wrote:
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
quoted
From: Victor Kamensky <redacted>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <hidden> Date: 2018-03-07 17:35:39
On Wed, 7 Mar 2018, Rob Landley wrote:
On 02/20/2018 12:56 PM, Stephen Smalley wrote:
quoted
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
quoted
From: Victor Kamensky <redacted>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <hidden> Date: 2018-03-11 03:07:22
On Tue, 20 Feb 2018, Stephen Smalley wrote:
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
quoted
From: Victor Kamensky <redacted>
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 <redacted>
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 <redacted>
---
security/selinux/hooks.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
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 <kamensky@cisco.com>
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 <kamensky@cisco.com>
---
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 <kamensky@cisco.com>
>> ---
>> 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. */
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Victor Kamensky <hidden> Date: 2018-03-11 03:08:18
On Tue, 20 Feb 2018, Stephen Smalley wrote:
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
quoted
From: Victor Kamensky <redacted>
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 <redacted>
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 <redacted>
---
security/selinux/hooks.c | 4 ++++
1 file changed, 4 insertions(+)
From: Stephen Smalley <hidden> Date: 2018-03-20 16:35:11
On 03/10/2018 10:07 PM, Victor Kamensky wrote:
quoted hunk
On Tue, 20 Feb 2018, Stephen Smalley wrote:
quoted
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
quoted
From: Victor Kamensky <redacted>
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:
quoted
From 6bf35bd055fdb12e94f3d5188eccfdbaa30dbcf4 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <redacted>
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 <redacted>
---
?security/selinux/hooks.c | 2 +-
?1 file changed, 1 insertion(+), 1 deletion(-)
???????? 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Stephen Smalley <hidden> Date: 2018-03-20 16:39:14
On 03/10/2018 10:08 PM, Victor Kamensky wrote:
On Tue, 20 Feb 2018, Stephen Smalley wrote:
quoted
On Fri, 2018-02-16 at 20:33 +0000, Taras Kondratiuk wrote:
quoted
From: Victor Kamensky <redacted>
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.
quoted
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.
quoted hunk
Sitll keeping logic in selinux_inode_post_setxattr, checked
that the following with much simple code works too:
quoted
From bfc54e4805f3059671417ff2cda1266bc68e18f9 Mon Sep 17 00:00:00 2001
From: Victor Kamensky <redacted>
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 <redacted>
---
?security/selinux/hooks.c | 4 ++++
?1 file changed, 4 insertions(+)
???????? 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"
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html