| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Y2lbEEtjhJwpbdRb@sol.localdomain>
Date: Mon, 7 Nov 2022 11:22:56 -0800
From: Eric Biggers <ebiggers@...nel.org>
To: "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
Cc: Theodore Ts'o <tytso@....edu>, linux-ext4@...r.kernel.org,
Harshad Shirwadkar <harshadshirwadkar@...il.com>,
Wang Shilong <wshilong@....com>,
Andreas Dilger <adilger.kernel@...ger.ca>,
Li Xi <lixi@....com>, Sebastien Buisson <sbuisson@....com>,
Maloo <maloo@...mcloud.com>, Li Dongyang <dongyangli@....com>,
Andreas Dilger <adilger@...mcloud.com>
Subject: Re: [RFCv1 67/72] sec: support encrypted files handling in pfsck mode
On Mon, Nov 07, 2022 at 05:51:55PM +0530, Ritesh Harjani (IBM) wrote:
> sec: support encrypted files handling in pfsck mode
>
"sec:" => "e2fsck:".
> +/**
> + * Search policy matching @policy in @info->policies
> + * @ctx: e2fsck context
> + * @info: encrypted_file_info to look into
> + * @policy: the policy we are looking for
> + * @parent: (out) last known parent, useful to insert a new leaf
> + * in @info->policies
> + *
> + * Return: id of found policy on success, -1 if no matching policy found.
> + */
> +static inline int search_policy(e2fsck_t ctx, struct encrypted_file_info *info,
> + union fscrypt_policy policy,
> + struct rb_node **parent)
> +{
> + struct rb_node *n = info->policies.rb_node;
> + struct policy_map_entry *entry;
> +
> + while (n) {
> + int res;
> +
> + *parent = n;
> + entry = ext2fs_rb_entry(n, struct policy_map_entry, node);
> + res = cmp_fscrypt_policies(ctx, &policy, &entry->policy);
> + if (res < 0)
> + n = n->rb_left;
> + else if (res > 0)
> + n = n->rb_right;
> + else
> + return entry->policy_id;
> + }
> + return -1;
> +}
Can this rbtree search code be reused by get_encryption_policy_id()?
Also, please use the existing constant NO_ENCRYPTION_POLICY instead of -1.
> +/*
> + * Merge @src encrypted info into @dest
> + */
> +int e2fsck_merge_encrypted_info(e2fsck_t ctx, struct encrypted_file_info *src,
> + struct encrypted_file_info *dest)
> +{
> + struct rb_root *src_policies = &src->policies;
> + __u32 *policy_trans;
> + int i, rc = 0;
> +
> + if (dest->file_ranges[src->file_ranges_count - 1].last_ino >
> + src->file_ranges[0].first_ino) {
There is an off-by-one error here. How about writing it like the following so
that it looks like the check in append_ino_and_policy_id():
if (src->file_ranges[0].first_ino <=
dest->file_ranges[src->file_ranges_count - 1].last_ino) {
> + /* First, deal with the encryption policy => ID map.
My understanding is that e2fsprogs follows the kernel coding style, where block
comments are formatted like the following:
/*
* text
*/
> + * Compare encryption policies in src with policies already recorded
> + * in dest. It can be similar policies, but recorded with a different
"similar" => "the same"
> + while (!ext2fs_rb_empty_root(src_policies)) {
> + struct policy_map_entry *entry, *newentry;
> + struct rb_node *new, *parent = NULL;
> + int existing_polid;
> +
> + entry = ext2fs_rb_entry(src_policies->rb_node,
> + struct policy_map_entry, node);
> + existing_polid = search_policy(ctx, dest,
> + entry->policy, &parent);
> + if (existing_polid >= 0) {
existing_polid != NO_ENCRYPTION_POLICY
> + /* The policy in src is already recorded in dest,
> + * so just update its id.
> + */
> + policy_trans[entry->policy_id] = existing_polid;
> + } else {
> + /* The policy in src is new to dest, so insert it
> + * with the next available id (its original id could
> + * be already used in dest).
> + */
> + rc = ext2fs_get_mem(sizeof(*newentry), &newentry);
> + if (rc)
> + goto out_merge;
Use handle_nomem() for memory allocation failures?
> + newentry->policy_id = dest->next_policy_id++;
> + newentry->policy = entry->policy;
> + ext2fs_rb_link_node(&newentry->node, parent, &new);
> + ext2fs_rb_insert_color(&newentry->node,
> + &dest->policies);
> + policy_trans[entry->policy_id] = newentry->policy_id;
This code also appears in get_encryption_policy_id(). Should it be refactored
into a helper function?
> + /* Second, deal with the inode number => encryption policy ID map. */
> + if (dest->file_ranges_capacity <
> + dest->file_ranges_count + src->file_ranges_count) {
> + /* dest->file_ranges is too short, increase its capacity. */
> + size_t new_capacity = dest->file_ranges_count +
> + src->file_ranges_count;
> +
> + /* Make sure we at least double the capacity. */
> + if (new_capacity < (dest->file_ranges_capacity * 2))
> + new_capacity = dest->file_ranges_capacity * 2;
> +
> + /* We won't need more than the filesystem's inode count. */
> + if (new_capacity > ctx->fs->super->s_inodes_count)
> + new_capacity = ctx->fs->super->s_inodes_count;
> +
> + rc = ext2fs_resize_mem(dest->file_ranges_capacity *
> + sizeof(struct encrypted_file_range),
> + new_capacity *
> + sizeof(struct encrypted_file_range),
> + &dest->file_ranges);
> + if (rc) {
> + fix_problem(ctx, PR_1_ALLOCATE_ENCRYPTED_INODE_LIST,
> + NULL);
> + /* Should never get here */
> + ctx->flags |= E2F_FLAG_ABORT;
> + goto out_merge;
> + }
> +
> + dest->file_ranges_capacity = new_capacity;
> + }
The exact allocation size that is needed is
'dest->file_ranges_count + src->file_ranges_count', so much of the above logic
is unnecessary. Just reallocate that exact amount.
Also, handle_nomem() should be used.
> + /* Copy file ranges from src to dest. */
> + for (i = 0; i < src->file_ranges_count; i++) {
> + /* Make sure to convert policy ids in src. */
> + src->file_ranges[i].policy_id =
> + policy_trans[src->file_ranges[i].policy_id];
> + dest->file_ranges[dest->file_ranges_count++] =
> + src->file_ranges[i];
> + }
This needs to handle UNRECOGNIZED_ENCRYPTION_POLICY as a special case.
Also, shouldn't src->file_ranges be freed after this?
> +
> +out_merge:
I guess the "_merge" in "out_merge:" comes from the name of the containing
function? It's a bit confusing. Maybe just use "out:".
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 7345c96d..e7dc017c 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -2411,9 +2411,6 @@ void e2fsck_pass1_run(e2fsck_t ctx)
> ctx->ea_block_quota_inodes = 0;
> }
>
> - /* We don't need the encryption policy => ID map any more */
> - destroy_encryption_policy_map(ctx);
With this change, there are no callers of destroy_encryption_policy_map()
outside encrypted_files.c. So absent any other changes,
destroy_encryption_policy_map() should be made into a static function and the
'if (info)' check inside it removed.
But, isn't there still some point where pass 1 is fully done and the encryption
policy ID map can be destroyed? Maybe e2fsck_pass1_merge_encrypted_info()
should be calling destroy_encryption_policy_map() on the global_ctx after doing
the merge?
- Eric
Powered by blists - more mailing lists