Re: [patch] Remove notion of key schemes

[Artem Bityutskiy]

On Thu, 2012-03-15 at 15:48 +0100, Joel Reardon wrote:
> @@ -112,8 +109,7 @@ static inline void ino_key_init_flash(const struct ubifs_info *c, void *k,
>  	union ubifs_key *key = k;
> 
>  	key->j32[0] = cpu_to_le32(inum);
> -	key->j32[1] = cpu_to_le32(UBIFS_INO_KEY << UBIFS_S_KEY_BLOCK_BITS);
> -	memset(k + 8, 0, UBIFS_MAX_KEY_LEN - 8);
> +	key->j32[1] = cpu_to_le32(UBIFS_INO_KEY << UBIFS_KEY_BLOCK_BITS);
>  }

So current UBIFS driver will always zero out unused parts of the key.
Looks like a flaw in UBIFS, but it is too late to do anything about
this. Could you please also think about the situation when a
security-enabled image is mounted in an older kernel which will start
zeroing unused bytes. What will happen when it is mounted by newer UBIFS
with the security stuff? Would be great to make sure this is handled
nicely.

>  /**
> - * key_max_inode_size - get maximum file size allowed by current key format.
> + * key_max_inode_size - get maximum file size allowed.
>   * @c: UBIFS file-system description object
>   */
>  static inline unsigned long long key_max_inode_size(const struct ubifs_info *c)
>  {
> -	switch (c->key_fmt) {
> -	case UBIFS_SIMPLE_KEY_FMT:
> -		return (1ULL << UBIFS_S_KEY_BLOCK_BITS) * UBIFS_BLOCK_SIZE;
> -	default:
> -		return 0;
> -	}
> +	return (1ULL << UBIFS_KEY_BLOCK_BITS) * UBIFS_BLOCK_SIZE;
>  }

I think this function should also be removed and turned into a macro. 
>  struct ubifs_dent_node {
>  	struct ubifs_ch ch;
> -	__u8 key[UBIFS_MAX_KEY_LEN];
> +	__u8 key[UBIFS_KEY_LEN];
> +	__u8 padding0[8]; /* Watch 'zero_dent_node_unused()' if changing! */
>  	__le64 inum;
> -	__u8 padding1;
> +	__u8 padding1; /* Watch 'zero_dent_node_unused()' if changing! */
>  	__u8 type;
>  	__le16 nlen;
>  	__u8 padding2[4]; /* Watch 'zero_dent_node_unused()' if changing! */
> @@ -552,7 +547,8 @@ struct ubifs_dent_node {
>   */
>  struct ubifs_data_node {
>  	struct ubifs_ch ch;
> -	__u8 key[UBIFS_MAX_KEY_LEN];
> +	__u8 key[UBIFS_KEY_LEN];
> +	__le64 crypto_lookup;

Err, no, this patch should be _pure_ key schemes removal. All the crypto
stuff should be separate.

Otherwise looks good!

-- 
Best Regards,
Artem Bityutskiy

Attachment: signature.asc
Description: This is a digitally signed message part