| Message ID | 1477054121-10198-26-git-send-email-richard@nod.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 21, 2016 at 02:48:40PM +0200, Richard Weinberger wrote: > @@ -190,6 +191,10 @@ long ubifs_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > sizeof(policy))) > return -EFAULT; > > + err = ubifs_enable_encryption(c); > + if (err) > + return err; > + > err = fscrypt_process_policy(file, &policy); Is ubifs_enable_encryption() being done with proper locking and authorization? As-is, anyone can call this at any time if they can open some file on UBIFS. FYI, the approach being taken for ext4 is that the encryption feature flag has to be explicitly turned on before FS_IOC_SET_ENCRYPTION_POLICY will work. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric, On 21.10.2016 20:30, Eric Biggers wrote: > On Fri, Oct 21, 2016 at 02:48:40PM +0200, Richard Weinberger wrote: >> @@ -190,6 +191,10 @@ long ubifs_ioctl(struct file *file, unsigned int cmd, unsigned long arg) >> sizeof(policy))) >> return -EFAULT; >> >> + err = ubifs_enable_encryption(c); >> + if (err) >> + return err; >> + >> err = fscrypt_process_policy(file, &policy); > > Is ubifs_enable_encryption() being done with proper locking and authorization? Anyone can encrypted files when UBIFS is build with that feature. Locking is fine since the operation is atomic. (UBI LEB change). > As-is, anyone can call this at any time if they can open some file on UBIFS. That's what I had in mind. > FYI, the approach being taken for ext4 is that the encryption feature flag has > to be explicitly turned on before FS_IOC_SET_ENCRYPTION_POLICY will work. So, on ext4 (f2fs too?) root has to enable to feature first before users can use it? IOW for ext4 the encryption flag means "file encryption is allowed", for UBIFS it means "at least one file got encrypted on this fs". I think I'll do what ext4 does to have common policies. BTW: Are these policies documented somewhere? Thanks, //richard -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 24, 2016 at 08:59:26AM +0200, Richard Weinberger wrote: > > So, on ext4 (f2fs too?) root has to enable to feature first before users > can use it? > IOW for ext4 the encryption flag means "file encryption is allowed", for UBIFS it > means "at least one file got encrypted on this fs". Historically there have been one or two feature flags which ext4 has enabled automatically, behind the system administrator's back. We've since decided that's a bad thing to do, and so it's a file system level policy not to do this. An example of why this can cause problems is if a system administrator boots a rescue CD that happens to use a newer kernel, and that newer kernel (plus potentially an updated userspace on said rescue CD) silently the feature flag to enable some feature (say, the first time we have files larger than 2GB, for example), and then when the system administrator books back into their RHEL or SLES kernel, they can no longer mount the file system. So ext4 has a policy now that we won't do that, since it's unfriendly to system administrators. That's a general rule, and not one which is specific to encryption. That way you can create a file system, and for the most part, modulo some really old, ancient flags which hopefully all Linux systems understand (I don't *think* there are any RHEL 2.x systems out there still :-), if a feature flag is not enabled, the kernel will return an error if you try to use some feature which is not enabled, instead of silently enabling it behind the user and/or sysadmin's back. I believe xfs may have a similar policy, and for similar reasons. (Actually, I'm not sure if xfs supports enabling features after the file system has been created at all; certainly Red Hat has a distro-level policy of not supporting features added post-mkfs, although I don't know how much that is enforced at the open source coding level. At one point there were some grumbles that Red Hat might want to disable some of tune2fs's features of enabling features post-mkfs time, but I think they've settled for just laughing at users who show up with bug reports after they converted a file system from ext3 to ext4. :-) > I think I'll do what ext4 does to have common policies. If you want to, feel free; but if there is good reason for Ubifs to want to be more flexible, I think there is room for differences at this level. The specific reason why we do this probably doesn't matter so much on embedded systems --- although I will note that many/most handset manufacturers have a policy of not supporting any new features that require new *kernel* fatures after the device has been released, because they don't want to deal with potential upgrade problems and device instabilities issues; users tend to get unhappy when their phones get bricked after an upgrade. Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ubifs/ioctl.c b/fs/ubifs/ioctl.c index 6bb5b35050de..3d10f5525274 100644 --- a/fs/ubifs/ioctl.c +++ b/fs/ubifs/ioctl.c @@ -183,6 +183,7 @@ long ubifs_ioctl(struct file *file, unsigned int cmd, unsigned long arg) } case FS_IOC_SET_ENCRYPTION_POLICY: { #ifdef CONFIG_UBIFS_FS_ENCRYPTION + struct ubifs_info *c = inode->i_sb->s_fs_info; struct fscrypt_policy policy; if (copy_from_user(&policy, @@ -190,6 +191,10 @@ long ubifs_ioctl(struct file *file, unsigned int cmd, unsigned long arg) sizeof(policy))) return -EFAULT; + err = ubifs_enable_encryption(c); + if (err) + return err; + err = fscrypt_process_policy(file, &policy); return err; diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c index 4a2b4c361587..54cef70ea16f 100644 --- a/fs/ubifs/sb.c +++ b/fs/ubifs/sb.c @@ -622,6 +622,16 @@ int ubifs_read_superblock(struct ubifs_info *c) c->big_lpt = !!(sup_flags & UBIFS_FLG_BIGLPT); c->space_fixup = !!(sup_flags & UBIFS_FLG_SPACE_FIXUP); c->double_hash = !!(sup_flags & UBIFS_FLG_DOUBLE_HASH); + c->encrypted = !!(sup_flags & UBIFS_FLG_ENCRYPTION); + +#ifndef CONFIG_UBIFS_FS_ENCRYPTION + if (c->encrypted) { + ubifs_err(c, "file system contains encrypted files but UBIFS" + " was built without crypto support."); + err = -EINVAL; + goto out; + } +#endif /* Automatically increase file system size to the maximum size */ c->old_leb_cnt = c->leb_cnt; @@ -809,3 +819,33 @@ int ubifs_fixup_free_space(struct ubifs_info *c) ubifs_msg(c, "free space fixup complete"); return err; } + +int ubifs_enable_encryption(struct ubifs_info *c) +{ + int err; + struct ubifs_sb_node *sup; + + if (c->encrypted) + return 0; + + if (c->ro_mount || c->ro_media) + return -EROFS; + + if (c->fmt_version < 5) { + ubifs_err(c, "on-flash format version 5 is needed for encryption"); + return -EINVAL; + } + + sup = ubifs_read_sb_node(c); + if (IS_ERR(sup)) + return PTR_ERR(sup); + + sup->flags |= cpu_to_le32(UBIFS_FLG_ENCRYPTION); + + err = ubifs_write_sb_node(c, sup); + if (!err) + c->encrypted = 1; + kfree(sup); + + return err; +} diff --git a/fs/ubifs/ubifs-media.h b/fs/ubifs/ubifs-media.h index 0cbdc6b70a00..bdc7935a5e41 100644 --- a/fs/ubifs/ubifs-media.h +++ b/fs/ubifs/ubifs-media.h @@ -420,11 +420,13 @@ enum { * UBIFS_FLG_SPACE_FIXUP: first-mount "fixup" of free space within LEBs needed * UBIFS_FLG_DOUBLE_HASH: store a 32bit cookie in directory entry nodes to * support 64bit cookies for lookups by hash + * UBIFS_FLG_ENCRYPTION: this filesystem contains encrypted files */ enum { UBIFS_FLG_BIGLPT = 0x02, UBIFS_FLG_SPACE_FIXUP = 0x04, UBIFS_FLG_DOUBLE_HASH = 0x08, + UBIFS_FLG_ENCRYPTION = 0x10, }; /** diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index 59d55887b3d8..85e2929aa6db 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -1015,6 +1015,7 @@ struct ubifs_debug_info; * @big_lpt: flag that LPT is too big to write whole during commit * @space_fixup: flag indicating that free space in LEBs needs to be cleaned up * @double_hash: flag indicating that we can do lookups by hash + * @encrypted: flag indicating that this file system contains encrypted files * @no_chk_data_crc: do not check CRCs when reading data nodes (except during * recovery) * @bulk_read: enable bulk-reads @@ -1258,6 +1259,7 @@ struct ubifs_info { unsigned int big_lpt:1; unsigned int space_fixup:1; unsigned int double_hash:1; + unsigned int encrypted:1; unsigned int no_chk_data_crc:1; unsigned int bulk_read:1; unsigned int default_compr:2; @@ -1651,6 +1653,7 @@ int ubifs_read_superblock(struct ubifs_info *c); struct ubifs_sb_node *ubifs_read_sb_node(struct ubifs_info *c); int ubifs_write_sb_node(struct ubifs_info *c, struct ubifs_sb_node *sup); int ubifs_fixup_free_space(struct ubifs_info *c); +int ubifs_enable_encryption(struct ubifs_info *c); /* replay.c */ int ubifs_validate_entry(struct ubifs_info *c,
This feature flag indicates that the filesystem contains encrypted files. Signed-off-by: Richard Weinberger <richard@nod.at> --- fs/ubifs/ioctl.c | 5 +++++ fs/ubifs/sb.c | 40 ++++++++++++++++++++++++++++++++++++++++ fs/ubifs/ubifs-media.h | 2 ++ fs/ubifs/ubifs.h | 3 +++ 4 files changed, 50 insertions(+)