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

[Matt Mackall <mpm@selenic.com>]

On Fri, Feb 27, 2004 at 05:05:01PM +0100, Christophe Saout wrote:
> 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.

I don't think this is a big deal either way, though my variant does
make it harder to do the wrong thing.
 
> > > > +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.

My original proposal was something like this. Downside with
_initializing_ on stack is that it's much more heavy-weight. We can
end having to sleep on pulling in an algorithm. The copy stuff,
hopefully, can be done in any context.
 
> What do you think?

I'd like to keep it to the minimal three new external functions until
we have a case that really demonstrates a need for the other stuff.
Let's keep it simple, get it merged, and go from there. The API I
posted will work for the three other users I'm aware of, if it works
for dm-crypt let's go with it.

I also want to hold off on adding ->copy until we find an algorithm
that can't be cleanly fixed not to need it.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting