UBIFS: introduce struct ubifs_keymap

This patch introduces struct ubifs_keymap, the entity responsible for key
generation, distribution, and lifetime management for ubifs's secure deletion.
In addition to the struct's (partial) definition, it is added as a field to
ubifs_info. The alloc/free routines are provided. More fields and functions
will come in following patches. As such, this introduces warnings about
defined but unused functions. (Since this goes in the joel branch, I suppose
thats okay for the time being and didn't add fake invocations.)

This is tested using local unit tests for the functions: alloc and deallocing,
checking the conversion of eb/off to pos and back is sound, and key generation
sets distinct keys for the keylength. UBIFS integck is also run to ensure
UBIFS is not effected.


Signed-off-by: Joel Reardon <reardonj@inf.ethz.ch>
---
 fs/ubifs/Makefile |    2 +-
 fs/ubifs/keymap.c |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/ubifs/ubifs.h  |   11 ++++
 3 files changed, 152 insertions(+), 1 deletions(-)
 create mode 100644 fs/ubifs/keymap.c

Hi,

On Mon, 2012-05-21 at 16:41 +0200, Joel Reardon wrote:
> +/*
> + * This file is part of UBIFS.
> + *
> + * Copyright (C) 2006-2008 Nokia Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc., 51
> + * Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> + *
> + * Authors: Adrian Hunter
> + *	    Artem Bityutskiy (Битюцкий Артём)
> + */

Please, add another copyright and different author names.

> +
> +/*
> + * The file implements the key storage, assignment, and deletion for UBIFS's
> + * secure deletion enhancement. For more information, read
> + * Documentation/filesystems/ubifsec.txt
> + */
> +
> +#include <linux/random.h>
> +#include "ubifs.h"
> +
> +#define RETURN_VAL_IF(v, e) do { if ((e)) return (v); } while (0)
> +#define RETURN_ON_ERR(e) do { if ((e)) return (e); } while (0)

In the Linux kernel we do not do things like this.

> +static void pos_to_eb_offset(const long long ksa_pos,
> +			     int *ksa_leb, int *ksa_offset)
> +{
> +	*ksa_offset = 0xffffffff & ksa_pos;
> +	*ksa_leb = 0xffffffff & (ksa_pos >> 32);
> +}

Throughout whole UBIFS we use shorter 'offs' for offsets - in variables
and function names. Please, be consistent with that and and use
'ksa_offs'.

> +
> +/**
> + * eb_offset_to_pos - convert a ksa_leb and offset to ksa_pos
> + * @ksa_leb: gets the KSA LEB number for the key position
> + * @ksa_offset: gets the KSA LEB offset for the key position
> + *
> + * This function converts the LEB number and offset for a key into the 64-bit
> + * key position value.
> + */
> +static long long eb_offset_to_pos(int ksa_leb, int ksa_offset)

I suggest different names: 'ksa_pos_couple()' (or 'ksa_pos_join()') and
'ksa_pos_decouple()', how does it sound to you?

> +{
> +	return (((long long) ksa_leb) << 32) + ksa_offset;

Sorry for being that picky, but for some reason in UBIFS we never have a
space between the cast and the identifier.

BTW, cast has higher priority that the shift, so you could have less
brackets, but not sure it would me more readable, so it is up to you.

> +}
> +
> +/**
> + * generate_key - generate a new encryption key
> + * @km: keymap structure to know the key size
> + * @to: key-size-sized buffer to hold the new key.
> + *
> + * This function generates a new random key and places it in the to field. The
> + * keymap stores the size of the key used as a field. We assume that
> + * get_random_bytes() will always yield cryptographically-suitable random
> + * data. If this ever changes, this will need to be updated.
> + */
> +static void generate_key(struct ubifs_keymap *km, u8 *to)
> +{
> +	get_random_bytes(to, km->key_size);
> +}
> +
> +/**
> + * keymap_init - construct and initialize a struct keymap
> + * @c: the ubifs info context to put the keymap into
> + *
> + * This function allocates a keymap data structure and initializes its fields.
> + * The ubifs_info context @c is passed as a parameter and its @km field is set
> + * to the keymap that is preprared by this function. If memory cannot be
> + * allocated, it returns -ENOMEM. Otherwise it returns 0.
> + */
> +int keymap_init(struct ubifs_info *c)
> +{
> +	struct ubifs_keymap *km;
> +
> +	c->km = NULL;

Useless assignment.

> +	km = kmalloc(sizeof(struct ubifs_keymap), GFP_NOFS);
> +	RETURN_VAL_IF(-ENOMEM, !km);

Please, do not do things like this in the linux kernel - we prefer to
open-code error checking for readability. Our eyes are not designed to
read weird constructs like this macro :-) Check how we allocate memory
in UBIFS and do it similarly. In general, try to follow the UBIFS style.

> +
> +	km->key_size = UBIFS_CRYPTO_KEYSIZE;
> +	km->keys_per_leb = c->leb_size / km->key_size;
> +	km->ksa_size = 0;
> +	km->ksa_first = 0;
> +	km->unused = 0;
> +	km->deleted = 0;

Do not set fields to zero - allocate the whole thing with kzalloc()
instead.

> +
> +	c->km = km;
> +	return 0;
> +}
> +
> +/**
> + * keymap_free - destruct and free memory used by a struct keymap
> + * @c: the ubifs info context that contanis the keymap
> + *
> + * This function frees the memory being used by the keymap.
> + */
> +void keymap_free(struct ubifs_info *c)
> +{
> +	kfree(c->km);
> +	c->km = NULL;

Please, kill this helper so far and do not set c->km to NULL. Call
kfree(c->km) directly from whenever you need. If it grows - you can
create a helper later.

> +}

If init/free are called only during 

> +/* keymap.c */
> +int keymap_init(struct ubifs_info *c);
> +void keymap_free(struct ubifs_info *c);
> +
> +/* keymap.c */
> +int keymap_init(struct ubifs_info *c);
> +void keymap_free(struct ubifs_info *c);

2 times the same?

On Mon, 2012-05-21 at 16:41 +0200, Joel Reardon wrote:
> +static void generate_key(struct ubifs_keymap *km, u8 *to)
> +{
> +       get_random_bytes(to, km->key_size);
> +}

If this function is not going to grow soon, just open-code
'get_random_bytes()' instead. Try to not introduce wrappers like this -
we do not appreciate it usually, we prefer open-coding things unless
there is a good reason to have a wrapper function.