| Message ID | 20181122154942.18262-1-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] fs: set xattrs in initramfs from regular files | expand |
On 11/22/2018 7:49 AM, Roberto Sassu wrote: > Although rootfs (tmpfs) supports xattrs, they are not set due to the > limitation of the cpio format. A new format called 'newcx' was proposed to > overcome this limitation. > > However, it looks like that adding a new format is not simple: 15 kernel > patches; user space tools must support the new format; mistakes made in the > past should be avoided; it is unclear whether the kernel should switch from > cpio to tar. > > The aim of this patch is to provide the same functionality without > introducing a new format. The value of xattrs is placed in regular files > having the same file name as the files xattrs are added to, plus a > separator and the xattr name (<filename>.xattr-<xattr name>). > > Example: > > '/bin/cat.xattr-security.ima' is the name of a file containing the value of > the security.ima xattr to be added to /bin/cat. > > At kernel initialization time, the kernel iterates over the rootfs > filesystem, and if it encounters files with the '.xattr-' separator, it > reads the content and adds the xattr to the file without the suffix. No. Really, no. It would be incredibly easy to use this mechanism to break into systems. > This proposal requires that LSMs and IMA allow the read and setxattr > operations. This should not be a concern since: files with xattr values > are not parsed by the kernel; user space processes are not yet executed. > > It would be possible to include all xattrs in the same file, but this > increases the risk of the kernel being compromised by parsing the content. The kernel mustn't do this. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > fs/Makefile | 2 +- > fs/readxattr.c | 171 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/fs.h | 2 + > init/main.c | 1 + > 4 files changed, 175 insertions(+), 1 deletion(-) > create mode 100644 fs/readxattr.c > > diff --git a/fs/Makefile b/fs/Makefile > index 293733f61594..738e1a4e4aff 100644 > --- a/fs/Makefile > +++ b/fs/Makefile > @@ -12,7 +12,7 @@ obj-y := open.o read_write.o file_table.o super.o \ > attr.o bad_inode.o file.o filesystems.o namespace.o \ > seq_file.o xattr.o libfs.o fs-writeback.o \ > pnode.o splice.o sync.o utimes.o d_path.o \ > - stack.o fs_struct.o statfs.o fs_pin.o nsfs.o > + stack.o fs_struct.o statfs.o fs_pin.o nsfs.o readxattr.o > > ifeq ($(CONFIG_BLOCK),y) > obj-y += buffer.o block_dev.o direct-io.o mpage.o > diff --git a/fs/readxattr.c b/fs/readxattr.c > new file mode 100644 > index 000000000000..01838f6f1e92 > --- /dev/null > +++ b/fs/readxattr.c > @@ -0,0 +1,171 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Huawei Technologies Duesseldorf GmbH > + * > + * Author: Roberto Sassu <roberto.sassu@huawei.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation, version 2 of the > + * License. > + * > + * File: readxattr.c > + * Read extended attributes from regular files in the initial ram disk > + */ > + > +#include <linux/kernel.h> > +#include <linux/export.h> > +#include <linux/xattr.h> > +#include <linux/file.h> > +#include <linux/cred.h> > +#include <linux/namei.h> > +#include <linux/fs.h> > + > +#include "internal.h" > + > +#define SETXATTR_FILENAME ".setxattr" > +#define FILENAME_XATTR_SEP ".xattr-" > + > + > +struct readdir_callback { > + struct dir_context ctx; > + struct path *path; > +}; > + > +LIST_HEAD(dir_list); > + > +struct dir_path { > + struct list_head next; > + struct path path; > +}; > + > +static int __init read_set_xattr(struct dir_context *__ctx, const char *name, > + int namelen, loff_t offset, u64 ino, > + unsigned int d_type) > +{ > + struct readdir_callback *ctx = container_of(__ctx, typeof(*ctx), ctx); > + struct path *dir = ctx->path, source_path, target_path; > + char filename[NAME_MAX + 1], *xattrname, *separator; > + struct dir_path *subdir; > + struct file *file; > + void *datap; > + loff_t size; > + int result; > + > + if (!strcmp(name, ".") || !strcmp(name, "..")) > + return 0; > + > + result = vfs_path_lookup(dir->dentry, dir->mnt, name, 0, &source_path); > + if (result) > + return 0; > + > + size = i_size_read(source_path.dentry->d_inode); > + if (size > XATTR_SIZE_MAX) > + goto out; > + > + if (source_path.dentry->d_inode->i_sb != dir->dentry->d_inode->i_sb) > + goto out; > + > + if (!S_ISREG(source_path.dentry->d_inode->i_mode) && > + !S_ISDIR(source_path.dentry->d_inode->i_mode)) > + goto out; > + > + if (S_ISREG(source_path.dentry->d_inode->i_mode)) { > + separator = strstr(name, FILENAME_XATTR_SEP); > + if (!separator) > + goto out; > + > + xattrname = separator + sizeof(FILENAME_XATTR_SEP) - 1; > + if (strlen(xattrname) > XATTR_NAME_MAX) > + goto out; > + } else { > + subdir = kmalloc(sizeof(*subdir), GFP_KERNEL); > + if (subdir) { > + subdir->path.dentry = source_path.dentry; > + subdir->path.mnt = source_path.mnt; > + > + list_add(&subdir->next, &dir_list); > + } > + > + return 0; > + } > + > + file = dentry_open(&source_path, O_RDONLY, current_cred()); > + if (IS_ERR(file)) > + goto out; > + > + result = kernel_read_file(file, &datap, &size, 0, READING_XATTR); > + if (result) > + goto out_fput; > + > + if (separator != name) { > + snprintf(filename, sizeof(filename), "%.*s", > + (int)(namelen - strlen(separator)), name); > + > + result = vfs_path_lookup(dir->dentry, dir->mnt, filename, 0, > + &target_path); > + if (result) > + goto out_vfree; > + > + inode_lock(target_path.dentry->d_inode); > + } else { > + target_path.dentry = dir->dentry; > + target_path.mnt = dir->mnt; > + } > + > + __vfs_setxattr_noperm(target_path.dentry, xattrname, datap, size, 0); > + > + if (separator != name) { > + inode_unlock(target_path.dentry->d_inode); > + path_put(&target_path); > + } > +out_vfree: > + vfree(datap); > +out_fput: > + fput(file); > +out: > + path_put(&source_path); > + return 0; > +} > + > +void __init set_xattrs_initrd(void) > +{ > + struct readdir_callback buf = { > + .ctx.actor = read_set_xattr, > + }; > + > + struct dir_path dir, *cur_dir; > + struct path path; > + struct file *file; > + int result; > + > + result = kern_path(SETXATTR_FILENAME, 0, &path); > + if (result) > + return; > + > + path_put(&path); > + > + result = kern_path("/", 0, &dir.path); > + if (result) > + return; > + > + list_add(&dir.next, &dir_list); > + > + while (!list_empty(&dir_list)) { > + cur_dir = list_first_entry(&dir_list, typeof(*cur_dir), next); > + > + file = dentry_open(&cur_dir->path, O_RDONLY, current_cred()); > + if (file) { > + buf.path = &cur_dir->path; > + iterate_dir(file, &buf.ctx); > + fput(file); > + } > + > + path_put(&cur_dir->path); > + list_del(&cur_dir->next); > + > + if (cur_dir != &dir) > + kfree(cur_dir); > + } > +} > +EXPORT_SYMBOL_GPL(set_xattrs_initrd); > diff --git a/include/linux/fs.h b/include/linux/fs.h > index c95c0807471f..b04edc1c32e9 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -2894,6 +2894,7 @@ extern int do_pipe_flags(int *, int); > id(KEXEC_INITRAMFS, kexec-initramfs) \ > id(POLICY, security-policy) \ > id(X509_CERTIFICATE, x509-certificate) \ > + id(XATTR, xattr) \ > id(MAX_ID, ) > > #define __fid_enumify(ENUM, dummy) READING_ ## ENUM, > @@ -3156,6 +3157,7 @@ const char *simple_get_link(struct dentry *, struct inode *, > extern const struct inode_operations simple_symlink_inode_operations; > > extern int iterate_dir(struct file *, struct dir_context *); > +extern void set_xattrs_initrd(void); > > extern int vfs_statx(int, const char __user *, int, struct kstat *, u32); > extern int vfs_statx_fd(unsigned int, struct kstat *, u32, unsigned int); > diff --git a/init/main.c b/init/main.c > index ee147103ba1b..a2f63bc8f9d4 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1180,5 +1180,6 @@ static noinline void __init kernel_init_freeable(void) > */ > > integrity_load_keys(); > + set_xattrs_initrd(); > load_default_modules(); > }
On Fri, 2018-11-23 at 11:03 -0800, Casey Schaufler wrote: > On 11/22/2018 7:49 AM, Roberto Sassu wrote: > > Although rootfs (tmpfs) supports xattrs, they are not set due to the > > limitation of the cpio format. A new format called 'newcx' was proposed to > > overcome this limitation. > > > > However, it looks like that adding a new format is not simple: 15 kernel > > patches; user space tools must support the new format; mistakes made in the > > past should be avoided; it is unclear whether the kernel should switch from > > cpio to tar. > > > > The aim of this patch is to provide the same functionality without > > introducing a new format. The value of xattrs is placed in regular files > > having the same file name as the files xattrs are added to, plus a > > separator and the xattr name (<filename>.xattr-<xattr name>). > > > > Example: > > > > '/bin/cat.xattr-security.ima' is the name of a file containing the value of > > the security.ima xattr to be added to /bin/cat. > > > > At kernel initialization time, the kernel iterates over the rootfs > > filesystem, and if it encounters files with the '.xattr-' separator, it > > reads the content and adds the xattr to the file without the suffix. > > No. > > Really, no. > > It would be incredibly easy to use this mechanism to break > into systems. > > > > This proposal requires that LSMs and IMA allow the read and setxattr > > operations. This should not be a concern since: files with xattr values > > are not parsed by the kernel; user space processes are not yet executed. > > > > It would be possible to include all xattrs in the same file, but this > > increases the risk of the kernel being compromised by parsing the content. > > The kernel mustn't do this. Mustn't do what? Store the xattr as separate detached files, include all the xattrs in a single or per security/LSM xattr attribute file(s), or either? Mimi
On 11/22/18 9:49 AM, Roberto Sassu wrote: > Although rootfs (tmpfs) supports xattrs, they are not set due to the > limitation of the cpio format. A new format called 'newcx' was proposed to > overcome this limitation. I got email about that format the day before you posted this, by the way. > However, it looks like that adding a new format is not simple: 15 kernel > patches; user space tools must support the new format; mistakes made in the > past should be avoided; it is unclear whether the kernel should switch from > cpio to tar. The kernel _can't_ switch from cpio to tar without breaking backwards compatability, it could only add tar as a second format it supported (remember cpio images can be sideloaded so a new rootfs can be used with an existing initramfs, plus existing build systems generate them and would still need to if they wanted to keep supporting older kernels), and then once you've got two formats somebody will propose zip support, and let's just not go there please. The changes to the userspace tools are trivial (I say that as the maintainer of toybox, which has a cpio). The argument was about things like 64 bit timestamps (y2038 problem), nanosecond support, sparse files, etc. And I think the argument had largely died down? Keep in mind the squashfs guy spent 5 years trying to get his filesystem merged (https://lwn.net/Articles/563578/), I spent several years trying to get my perl removal patch merged (and only work up the enthusiasm to resubmit http://lists.busybox.net/pipermail/buildroot/2015-March/123385.html https://patchwork.kernel.org/patch/9193529/ https://lkml.org/lkml/2017/9/13/651 about once a year because dealing with linux-kernel is just no fun for hobbyists anymore). > The aim of this patch is to provide the same functionality without > introducing a new format. The value of xattrs is placed in regular files > having the same file name as the files xattrs are added to, plus a > separator and the xattr name (<filename>.xattr-<xattr name>). I think you're solving the wrong problem, but that's just my opinion. Rob
On 11/23/2018 11:30 AM, Mimi Zohar wrote: > On Fri, 2018-11-23 at 11:03 -0800, Casey Schaufler wrote: >> On 11/22/2018 7:49 AM, Roberto Sassu wrote: >>> Although rootfs (tmpfs) supports xattrs, they are not set due to the >>> limitation of the cpio format. A new format called 'newcx' was proposed to >>> overcome this limitation. >>> >>> However, it looks like that adding a new format is not simple: 15 kernel >>> patches; user space tools must support the new format; mistakes made in the >>> past should be avoided; it is unclear whether the kernel should switch from >>> cpio to tar. >>> >>> The aim of this patch is to provide the same functionality without >>> introducing a new format. The value of xattrs is placed in regular files >>> having the same file name as the files xattrs are added to, plus a >>> separator and the xattr name (<filename>.xattr-<xattr name>). >>> >>> Example: >>> >>> '/bin/cat.xattr-security.ima' is the name of a file containing the value of >>> the security.ima xattr to be added to /bin/cat. >>> >>> At kernel initialization time, the kernel iterates over the rootfs >>> filesystem, and if it encounters files with the '.xattr-' separator, it >>> reads the content and adds the xattr to the file without the suffix. >> No. >> >> Really, no. >> >> It would be incredibly easy to use this mechanism to break >> into systems. >> >> >>> This proposal requires that LSMs and IMA allow the read and setxattr >>> operations. This should not be a concern since: files with xattr values >>> are not parsed by the kernel; user space processes are not yet executed. >>> >>> It would be possible to include all xattrs in the same file, but this >>> increases the risk of the kernel being compromised by parsing the content. >> The kernel mustn't do this. > Mustn't do what? Store the xattr as separate detached files, > include all the xattrs in a single or per security/LSM xattr attribute > file(s), or either? Any and all of the above. The proposed behavior is a kludge around making the installation tools work correctly. Sure, it may be easier to change the kernel than to change the utilities. That's doesn't make it right. > > Mimi > >
On Fri, 2018-11-23 at 18:07 -0800, Casey Schaufler wrote: > On 11/23/2018 11:30 AM, Mimi Zohar wrote: > > On Fri, 2018-11-23 at 11:03 -0800, Casey Schaufler wrote: > >> On 11/22/2018 7:49 AM, Roberto Sassu wrote: > >>> Although rootfs (tmpfs) supports xattrs, they are not set due to the > >>> limitation of the cpio format. A new format called 'newcx' was proposed to > >>> overcome this limitation. > >>> > >>> However, it looks like that adding a new format is not simple: 15 kernel > >>> patches; user space tools must support the new format; mistakes made in the > >>> past should be avoided; it is unclear whether the kernel should switch from > >>> cpio to tar. > >>> > >>> The aim of this patch is to provide the same functionality without > >>> introducing a new format. The value of xattrs is placed in regular files > >>> having the same file name as the files xattrs are added to, plus a > >>> separator and the xattr name (<filename>.xattr-<xattr name>). > >>> > >>> Example: > >>> > >>> '/bin/cat.xattr-security.ima' is the name of a file containing the value of > >>> the security.ima xattr to be added to /bin/cat. > >>> > >>> At kernel initialization time, the kernel iterates over the rootfs > >>> filesystem, and if it encounters files with the '.xattr-' separator, it > >>> reads the content and adds the xattr to the file without the suffix. > >> No. > >> > >> Really, no. > >> > >> It would be incredibly easy to use this mechanism to break > >> into systems. Assuming that the initramfs itself was signed, how? > >> > >>> This proposal requires that LSMs and IMA allow the read and setxattr > >>> operations. This should not be a concern since: files with xattr values > >>> are not parsed by the kernel; user space processes are not yet executed. > >>> > >>> It would be possible to include all xattrs in the same file, but this > >>> increases the risk of the kernel being compromised by parsing the content. > >> The kernel mustn't do this. > > Mustn't do what? Store the xattr as separate detached files, > > include all the xattrs in a single or per security/LSM xattr attribute > > file(s), or either? > > Any and all of the above. The proposed behavior is a kludge > around making the installation tools work correctly. Sure, it > may be easier to change the kernel than to change the utilities. > That's doesn't make it right. Modifying userspace tools, as Rob Landley pointed out in terms of toybox, isn't difficult. The difficulty has been in reviewing and upstreaming the kernel CPIO changes. This patch was posted in order to address the lack of xattr support in the initramfs. Before totally dismissing this or a similar solution, is there a safe method for including the xattrs? Would defining an LSM hook here help? Each LSM would define its own method for storing and applying, or restoring, xattr labels. Mimi
On 11/23/2018 9:21 PM, Rob Landley wrote: > On 11/22/18 9:49 AM, Roberto Sassu wrote: >> Although rootfs (tmpfs) supports xattrs, they are not set due to the >> limitation of the cpio format. A new format called 'newcx' was proposed to >> overcome this limitation. > > I got email about that format the day before you posted this, by the way. > >> However, it looks like that adding a new format is not simple: 15 kernel >> patches; user space tools must support the new format; mistakes made in the >> past should be avoided; it is unclear whether the kernel should switch from >> cpio to tar. > > The kernel _can't_ switch from cpio to tar without breaking backwards > compatability, it could only add tar as a second format it supported (remember > cpio images can be sideloaded so a new rootfs can be used with an existing > initramfs, plus existing build systems generate them and would still need to if > they wanted to keep supporting older kernels), and then once you've got two > formats somebody will propose zip support, and let's just not go there please. > > The changes to the userspace tools are trivial (I say that as the maintainer of > toybox, which has a cpio). The argument was about things like 64 bit timestamps > (y2038 problem), nanosecond support, sparse files, etc. And I think the argument > had largely died down? > > Keep in mind the squashfs guy spent 5 years trying to get his filesystem merged > (https://lwn.net/Articles/563578/), I spent several years trying to get my perl > removal patch merged (and only work up the enthusiasm to resubmit > http://lists.busybox.net/pipermail/buildroot/2015-March/123385.html > https://patchwork.kernel.org/patch/9193529/ https://lkml.org/lkml/2017/9/13/651 > about once a year because dealing with linux-kernel is just no fun for hobbyists > anymore). > >> The aim of this patch is to provide the same functionality without >> introducing a new format. The value of xattrs is placed in regular files >> having the same file name as the files xattrs are added to, plus a >> separator and the xattr name (<filename>.xattr-<xattr name>). > > I think you're solving the wrong problem, but that's just my opinion. Instead of iterating over rootfs, would it be better to detect files with extended attributes (from the file name) when the cpio image is parsed by the kernel, and call sys_lsetxattr() in do_copy()? This part can be turned on by introducing a new type in the existing format (if possible). The impact of this alternative is very low, and LSMs/IMA would be able, with minimum effort, to enforce policies on files in the initial ram disk. Roberto > Rob >
On 11/26/2018 4:51 AM, Mimi Zohar wrote: > On Fri, 2018-11-23 at 18:07 -0800, Casey Schaufler wrote: >> On 11/23/2018 11:30 AM, Mimi Zohar wrote: >>> On Fri, 2018-11-23 at 11:03 -0800, Casey Schaufler wrote: >>>> On 11/22/2018 7:49 AM, Roberto Sassu wrote: >>>>> Although rootfs (tmpfs) supports xattrs, they are not set due to the >>>>> limitation of the cpio format. A new format called 'newcx' was proposed to >>>>> overcome this limitation. >>>>> >>>>> However, it looks like that adding a new format is not simple: 15 kernel >>>>> patches; user space tools must support the new format; mistakes made in the >>>>> past should be avoided; it is unclear whether the kernel should switch from >>>>> cpio to tar. >>>>> >>>>> The aim of this patch is to provide the same functionality without >>>>> introducing a new format. The value of xattrs is placed in regular files >>>>> having the same file name as the files xattrs are added to, plus a >>>>> separator and the xattr name (<filename>.xattr-<xattr name>). >>>>> >>>>> Example: >>>>> >>>>> '/bin/cat.xattr-security.ima' is the name of a file containing the value of >>>>> the security.ima xattr to be added to /bin/cat. >>>>> >>>>> At kernel initialization time, the kernel iterates over the rootfs >>>>> filesystem, and if it encounters files with the '.xattr-' separator, it >>>>> reads the content and adds the xattr to the file without the suffix. >>>> No. >>>> >>>> Really, no. >>>> >>>> It would be incredibly easy to use this mechanism to break >>>> into systems. > Assuming that the initramfs itself was signed, how? I don't share your faith in signatures. >>>>> This proposal requires that LSMs and IMA allow the read and setxattr >>>>> operations. This should not be a concern since: files with xattr values >>>>> are not parsed by the kernel; user space processes are not yet executed. >>>>> >>>>> It would be possible to include all xattrs in the same file, but this >>>>> increases the risk of the kernel being compromised by parsing the content. >>>> The kernel mustn't do this. >>> Mustn't do what? Store the xattr as separate detached files, >>> include all the xattrs in a single or per security/LSM xattr attribute >>> file(s), or either? >> Any and all of the above. The proposed behavior is a kludge >> around making the installation tools work correctly. Sure, it >> may be easier to change the kernel than to change the utilities. >> That's doesn't make it right. > Modifying userspace tools, as Rob Landley pointed out in terms of > toybox, isn't difficult. The difficulty has been in reviewing and > upstreaming the kernel CPIO changes. No sympathy from me there. And why wouldn't this scheme require just as much review? > This patch was posted in order to address the lack of xattr support in > the initramfs. Before totally dismissing this or a similar solution, > is there a safe method for including the xattrs? The extension to CPIO sounds right to me. > Would defining an LSM hook here help? Each LSM would define its own > method for storing and applying, or restoring, xattr labels. I'm more concerned about how this could be used with file capabilities than I am with access control attributes. I don't see how adding a hook for this special case would help. > Mimi
On 11/26/2018 4:56 AM, Roberto Sassu wrote: > On 11/23/2018 9:21 PM, Rob Landley wrote: >> On 11/22/18 9:49 AM, Roberto Sassu wrote: >>> Although rootfs (tmpfs) supports xattrs, they are not set due to the >>> limitation of the cpio format. A new format called 'newcx' was proposed to >>> overcome this limitation. >> >> I got email about that format the day before you posted this, by the way. >> >>> However, it looks like that adding a new format is not simple: 15 kernel >>> patches; user space tools must support the new format; mistakes made in the >>> past should be avoided; it is unclear whether the kernel should switch from >>> cpio to tar. >> >> The kernel _can't_ switch from cpio to tar without breaking backwards >> compatability, it could only add tar as a second format it supported (remember >> cpio images can be sideloaded so a new rootfs can be used with an existing >> initramfs, plus existing build systems generate them and would still need to if >> they wanted to keep supporting older kernels), and then once you've got two >> formats somebody will propose zip support, and let's just not go there please. >> >> The changes to the userspace tools are trivial (I say that as the maintainer of >> toybox, which has a cpio). The argument was about things like 64 bit timestamps >> (y2038 problem), nanosecond support, sparse files, etc. And I think the argument >> had largely died down? >> >> Keep in mind the squashfs guy spent 5 years trying to get his filesystem merged >> (https://lwn.net/Articles/563578/), I spent several years trying to get my perl >> removal patch merged (and only work up the enthusiasm to resubmit >> http://lists.busybox.net/pipermail/buildroot/2015-March/123385.html >> https://patchwork.kernel.org/patch/9193529/ https://lkml.org/lkml/2017/9/13/651 >> about once a year because dealing with linux-kernel is just no fun for hobbyists >> anymore). >> >>> The aim of this patch is to provide the same functionality without >>> introducing a new format. The value of xattrs is placed in regular files >>> having the same file name as the files xattrs are added to, plus a >>> separator and the xattr name (<filename>.xattr-<xattr name>). >> >> I think you're solving the wrong problem, but that's just my opinion. > > Instead of iterating over rootfs, would it be better to detect files > with extended attributes (from the file name) when the cpio image is > parsed by the kernel, and call sys_lsetxattr() in do_copy()? This part > can be turned on by introducing a new type in the existing format (if > possible). A very similar approach was used in at least one MLS Unix system back in the day. It used tar, but would have worked just as well with CPIO. Any file with a specific name was assumed to contain the security attributes for the preceding file, and tar invoked a helper program to set them. No change to the tar format was required, and if you read an archive with a generic tar you just got multiple entries for the special name. No format or special types required. > > The impact of this alternative is very low, and LSMs/IMA would be able, > with minimum effort, to enforce policies on files in the initial ram > disk. True. And it worked. But it was still a kludge.
On 11/26/18 6:56 AM, Roberto Sassu wrote: > On 11/23/2018 9:21 PM, Rob Landley wrote: >>> The aim of this patch is to provide the same functionality without >>> introducing a new format. The value of xattrs is placed in regular files >>> having the same file name as the files xattrs are added to, plus a >>> separator and the xattr name (<filename>.xattr-<xattr name>). >> >> I think you're solving the wrong problem, but that's just my opinion. > > Instead of iterating over rootfs, would it be better to detect files > with extended attributes (from the file name) when the cpio image is > parsed by the kernel, Huh, I thought at first glance that's what the new approach _was_ doing. In band signaling in the archive is ugly, still requires new tools to create it, doesn't address the y2038 issue... (Although we could cheat, treat the time signature as unsigned, and buy another few decades.) But doing that in the filesystem _after_ you extract the archive is beyond ugly. > and call sys_lsetxattr() in do_copy()? This part > can be turned on by introducing a new type in the existing format (if > possible). > > The impact of this alternative is very low, and LSMs/IMA would be able, > with minimum effort, to enforce policies on files in the initial ram > disk. The cpio extension isn't a big deal, I was pondering doing it myself in toybox (and submitting a kernel patch to consume the output) before Mimi approached me. (I did the initmpfs stuff already, I've stomped around in this area before.) I just didn't because mimi sent their patch first, so I waited for that to work its way though. Unfortunately, it's simple enough that there was a bit of bikeshedding. (You can store time in milliseconds as a 64 bit number without worrying about the range, but if you do it as nanoseconds you need two fields, but people spoke up and said "but if you don't store the nanoseconds the rounding causes spurious time differences when between filesystems and it confuses our build system about what has and hasn't changed"...) The new in-band signaling proposal is, at best, a hack. (Filename lengths are capped at 255 in the VFS, can you strip the xattrs on a long filename by having the extension fail to create in the filesystem? Or do you have an arbitrary max length on filenames because you need to reserve room for the extension?) Rob
On 11/26/2018 6:42 PM, Rob Landley wrote: > On 11/26/18 6:56 AM, Roberto Sassu wrote: >> On 11/23/2018 9:21 PM, Rob Landley wrote: >>>> The aim of this patch is to provide the same functionality without >>>> introducing a new format. The value of xattrs is placed in regular files >>>> having the same file name as the files xattrs are added to, plus a >>>> separator and the xattr name (<filename>.xattr-<xattr name>). >>> >>> I think you're solving the wrong problem, but that's just my opinion. >> >> Instead of iterating over rootfs, would it be better to detect files >> with extended attributes (from the file name) when the cpio image is >> parsed by the kernel, > > Huh, I thought at first glance that's what the new approach _was_ doing. > > In band signaling in the archive is ugly, still requires new tools to create it, For SElinux, the changes would be minimal. Instead of adding the xattr, setfiles would create a regular file with the suffix, in the temporary directory containing the files to be added to the CPIO image. For IMA, I think there is also a tool to write file signatures. It shouldn't be a problem to do the same modification I proposed for SELinux. > doesn't address the y2038 issue... (Although we could cheat, treat the time > signature as unsigned, and buy another few decades.) > > But doing that in the filesystem _after_ you extract the archive is beyond ugly. > >> and call sys_lsetxattr() in do_copy()? This part >> can be turned on by introducing a new type in the existing format (if >> possible). >> >> The impact of this alternative is very low, and LSMs/IMA would be able, >> with minimum effort, to enforce policies on files in the initial ram >> disk. > > The cpio extension isn't a big deal, I was pondering doing it myself in toybox > (and submitting a kernel patch to consume the output) before Mimi approached me. > (I did the initmpfs stuff already, I've stomped around in this area before.) I > just didn't because mimi sent their patch first, so I waited for that to work > its way though. > > Unfortunately, it's simple enough that there was a bit of bikeshedding. (You can > store time in milliseconds as a 64 bit number without worrying about the range, > but if you do it as nanoseconds you need two fields, but people spoke up and > said "but if you don't store the nanoseconds the rounding causes spurious time > differences when between filesystems and it confuses our build system about what > has and hasn't changed"...) > > The new in-band signaling proposal is, at best, a hack. (Filename lengths are > capped at 255 in the VFS, can you strip the xattrs on a long filename by having > the extension fail to create in the filesystem? Or do you have an arbitrary max > length on filenames because you need to reserve room for the extension?) Yes, that would be a limitation. Alternatively, files with xattrs could be placed in a subdir. For example: /bin/cat /bin/.xattr-<xattr name>/cat Roberto > Rob >
diff --git a/fs/Makefile b/fs/Makefile index 293733f61594..738e1a4e4aff 100644 --- a/fs/Makefile +++ b/fs/Makefile @@ -12,7 +12,7 @@ obj-y := open.o read_write.o file_table.o super.o \ attr.o bad_inode.o file.o filesystems.o namespace.o \ seq_file.o xattr.o libfs.o fs-writeback.o \ pnode.o splice.o sync.o utimes.o d_path.o \ - stack.o fs_struct.o statfs.o fs_pin.o nsfs.o + stack.o fs_struct.o statfs.o fs_pin.o nsfs.o readxattr.o ifeq ($(CONFIG_BLOCK),y) obj-y += buffer.o block_dev.o direct-io.o mpage.o diff --git a/fs/readxattr.c b/fs/readxattr.c new file mode 100644 index 000000000000..01838f6f1e92 --- /dev/null +++ b/fs/readxattr.c @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 Huawei Technologies Duesseldorf GmbH + * + * Author: Roberto Sassu <roberto.sassu@huawei.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation, version 2 of the + * License. + * + * File: readxattr.c + * Read extended attributes from regular files in the initial ram disk + */ + +#include <linux/kernel.h> +#include <linux/export.h> +#include <linux/xattr.h> +#include <linux/file.h> +#include <linux/cred.h> +#include <linux/namei.h> +#include <linux/fs.h> + +#include "internal.h" + +#define SETXATTR_FILENAME ".setxattr" +#define FILENAME_XATTR_SEP ".xattr-" + + +struct readdir_callback { + struct dir_context ctx; + struct path *path; +}; + +LIST_HEAD(dir_list); + +struct dir_path { + struct list_head next; + struct path path; +}; + +static int __init read_set_xattr(struct dir_context *__ctx, const char *name, + int namelen, loff_t offset, u64 ino, + unsigned int d_type) +{ + struct readdir_callback *ctx = container_of(__ctx, typeof(*ctx), ctx); + struct path *dir = ctx->path, source_path, target_path; + char filename[NAME_MAX + 1], *xattrname, *separator; + struct dir_path *subdir; + struct file *file; + void *datap; + loff_t size; + int result; + + if (!strcmp(name, ".") || !strcmp(name, "..")) + return 0; + + result = vfs_path_lookup(dir->dentry, dir->mnt, name, 0, &source_path); + if (result) + return 0; + + size = i_size_read(source_path.dentry->d_inode); + if (size > XATTR_SIZE_MAX) + goto out; + + if (source_path.dentry->d_inode->i_sb != dir->dentry->d_inode->i_sb) + goto out; + + if (!S_ISREG(source_path.dentry->d_inode->i_mode) && + !S_ISDIR(source_path.dentry->d_inode->i_mode)) + goto out; + + if (S_ISREG(source_path.dentry->d_inode->i_mode)) { + separator = strstr(name, FILENAME_XATTR_SEP); + if (!separator) + goto out; + + xattrname = separator + sizeof(FILENAME_XATTR_SEP) - 1; + if (strlen(xattrname) > XATTR_NAME_MAX) + goto out; + } else { + subdir = kmalloc(sizeof(*subdir), GFP_KERNEL); + if (subdir) { + subdir->path.dentry = source_path.dentry; + subdir->path.mnt = source_path.mnt; + + list_add(&subdir->next, &dir_list); + } + + return 0; + } + + file = dentry_open(&source_path, O_RDONLY, current_cred()); + if (IS_ERR(file)) + goto out; + + result = kernel_read_file(file, &datap, &size, 0, READING_XATTR); + if (result) + goto out_fput; + + if (separator != name) { + snprintf(filename, sizeof(filename), "%.*s", + (int)(namelen - strlen(separator)), name); + + result = vfs_path_lookup(dir->dentry, dir->mnt, filename, 0, + &target_path); + if (result) + goto out_vfree; + + inode_lock(target_path.dentry->d_inode); + } else { + target_path.dentry = dir->dentry; + target_path.mnt = dir->mnt; + } + + __vfs_setxattr_noperm(target_path.dentry, xattrname, datap, size, 0); + + if (separator != name) { + inode_unlock(target_path.dentry->d_inode); + path_put(&target_path); + } +out_vfree: + vfree(datap); +out_fput: + fput(file); +out: + path_put(&source_path); + return 0; +} + +void __init set_xattrs_initrd(void) +{ + struct readdir_callback buf = { + .ctx.actor = read_set_xattr, + }; + + struct dir_path dir, *cur_dir; + struct path path; + struct file *file; + int result; + + result = kern_path(SETXATTR_FILENAME, 0, &path); + if (result) + return; + + path_put(&path); + + result = kern_path("/", 0, &dir.path); + if (result) + return; + + list_add(&dir.next, &dir_list); + + while (!list_empty(&dir_list)) { + cur_dir = list_first_entry(&dir_list, typeof(*cur_dir), next); + + file = dentry_open(&cur_dir->path, O_RDONLY, current_cred()); + if (file) { + buf.path = &cur_dir->path; + iterate_dir(file, &buf.ctx); + fput(file); + } + + path_put(&cur_dir->path); + list_del(&cur_dir->next); + + if (cur_dir != &dir) + kfree(cur_dir); + } +} +EXPORT_SYMBOL_GPL(set_xattrs_initrd); diff --git a/include/linux/fs.h b/include/linux/fs.h index c95c0807471f..b04edc1c32e9 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2894,6 +2894,7 @@ extern int do_pipe_flags(int *, int); id(KEXEC_INITRAMFS, kexec-initramfs) \ id(POLICY, security-policy) \ id(X509_CERTIFICATE, x509-certificate) \ + id(XATTR, xattr) \ id(MAX_ID, ) #define __fid_enumify(ENUM, dummy) READING_ ## ENUM, @@ -3156,6 +3157,7 @@ const char *simple_get_link(struct dentry *, struct inode *, extern const struct inode_operations simple_symlink_inode_operations; extern int iterate_dir(struct file *, struct dir_context *); +extern void set_xattrs_initrd(void); extern int vfs_statx(int, const char __user *, int, struct kstat *, u32); extern int vfs_statx_fd(unsigned int, struct kstat *, u32, unsigned int); diff --git a/init/main.c b/init/main.c index ee147103ba1b..a2f63bc8f9d4 100644 --- a/init/main.c +++ b/init/main.c @@ -1180,5 +1180,6 @@ static noinline void __init kernel_init_freeable(void) */ integrity_load_keys(); + set_xattrs_initrd(); load_default_modules(); }
Although rootfs (tmpfs) supports xattrs, they are not set due to the limitation of the cpio format. A new format called 'newcx' was proposed to overcome this limitation. However, it looks like that adding a new format is not simple: 15 kernel patches; user space tools must support the new format; mistakes made in the past should be avoided; it is unclear whether the kernel should switch from cpio to tar. The aim of this patch is to provide the same functionality without introducing a new format. The value of xattrs is placed in regular files having the same file name as the files xattrs are added to, plus a separator and the xattr name (<filename>.xattr-<xattr name>). Example: '/bin/cat.xattr-security.ima' is the name of a file containing the value of the security.ima xattr to be added to /bin/cat. At kernel initialization time, the kernel iterates over the rootfs filesystem, and if it encounters files with the '.xattr-' separator, it reads the content and adds the xattr to the file without the suffix. This proposal requires that LSMs and IMA allow the read and setxattr operations. This should not be a concern since: files with xattr values are not parsed by the kernel; user space processes are not yet executed. It would be possible to include all xattrs in the same file, but this increases the risk of the kernel being compromised by parsing the content. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- fs/Makefile | 2 +- fs/readxattr.c | 171 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/fs.h | 2 + init/main.c | 1 + 4 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 fs/readxattr.c