Re: [PATCH/proposal] dm-crypt: add digest-based iv generation mode

[Christophe Saout <christophe@saout.de>]

Am Do, den 26.02.2004 schrieb Matt Mackall um 21:02:

> User is giving us the size of his buffer, not the size of the tfm
> which we already know. We refuse to copy if buffer is not big enough,
> otherwise return number of bytes copied.

Well, I would usually except the user knows what he does, but okay, if
you think that's safer. It requires the user to carry the size of the
buffer around. Assuming he kmallocs the buffer in one function with the
correct size and wants to use it in another function (a mempool or
something, who knows). He doesn't know the size of the buffer there.

> This may seem a little
> redundant for the on-stack usage of the API, but may make sense in
> other cases.

It may, yes. But I don't think this kind of thing is done elsewhere in
the kernel. It's okay for things like user space libraries where the
libraries and users can be compiled separately to catch problems with
ABI changes, but in the kernel? I think it's overkill.
 
These things should be caught using BUG_ONs if you thing someone might
get them wrong somehow und in the future if something changes. But now
if they add additional parameters.

> > > +void crypto_cleanup_copy_tfm(char *user_tfm)
> > > +{
> > > +	crypto_exit_ops((struct crypto_tfm *)user_tfm);
> > 
> > This looks dangerous. The algorithm might free a buffer. This is only
> > safe if we introduce per-algorithm copy methods that also duplicate
> > external buffers.
> 
> I'm currently working under the assumption that such external buffers
> are unnecessary but I haven't done the audit. If and when such code
> exist, such code should be added, yes. Hence the comment in the copy
> function:
> 
> +       /* currently assumes shallow copy is sufficient */

Ok, I see.

We could add some functions so that everything is symmetric:
(the names with a star are already existing)

*crypto_alloc_tfm
\_ crypto_init_tfm

*crypto_free_tfm
\_ crypto_release_tfm

crypto_clone_tfm
\_ crypto_copy_tfm

crypto_get_alg_size
\_crypto_get_tfm_size

crypto_init_tfm does everything but the kmalloc
crypto_release_tfm everything but the kfree

So crypto_alloc_tfm and crypto_release_tfm can be changed to call
crypto_init_fdm and crypto_release_tfm plus crypto_get_alg_size/kmalloc
and kfree.

crypto_clone_tfm calls crypto_get_tfm_size, kmalloc and crypto_copy_tfm.
crypto_copy_tfm copies the tfm structure, cares about algorithm
reference counting and calls a (new) copy method. This copy method
should copy things in its context like kmalloc'ed structures (or
increment a reference count if it's a static memory structure or
something). (however kmalloc's should be avoided if possible, the
variable sized context provides some flexibility)

crypto_get_alg_size returns the size of the tfm structure when algorithm
name and flags are given, crypto_get_tfm size returns the size of an
existing tfm structure.

So we can also directly initialize a tfm structure on a stack, not only
copy it to a stack. Very flexible.

What do you think?