On Sun, Feb 14, 2010 at 09:42:54PM +0100, Sebastian Andrzej Siewior wrote:
> * Sebastian Andrzej Siewior | 2010-02-12 09:42:28 [+0100]:
>
> >+static void arc4_ivsetup(struct arc4_ctx *ctx, u8 *iv)
> > {
> >- struct arc4_ctx *ctx = crypto_tfm_ctx(tfm);
> >+ if (unlikely(!ctx->new_key))
>
> That should be likely(). Do you want me resend the whole thing? Haven't
> noticed anything else

How about we just remove it? It's not on a hot path anyway.

I can do this when integrating the patch so you don't have to
resend.

Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Fri, Feb 12, 2010 at 09:42:28AM +0100, Sebastian Andrzej Siewior wrote:
>
> -static void arc4_crypt(struct crypto_tfm *tfm, u8 *out, const u8 *in)
> +static void arc4_ivsetup(struct arc4_ctx *ctx, u8 *iv)
> {
> - struct arc4_ctx *ctx = crypto_tfm_ctx(tfm);
> + if (unlikely(!ctx->new_key))
> + return;
> + memcpy(iv, &ctx->iv, sizeof(ctx->iv));
> + ctx->new_key = 0;

Sorry, but this doesn't work.

A ctx is supposed to be reentrant. That is, while one thread
is working away with a given ctx I should be able to use that
same ctx in a different thread without them clobbering each
other.

So that means (in general) you must not modify the ctx in any
function other than setkey.

This also brings up the bigger question of how we transition to
this new arc4. I don't think we need to maintain exactly the
same behaviour as the existing ecb(arc4).

So what we could do is simply add a new blkcipher arc4, alongside
the existing cipher arc4. Then we can convert the existing users
across, and finally remove the old arc4.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Sun, Feb 21, 2010 at 09:01:40PM +0100, Sebastian Andrzej Siewior wrote:
>
> I also destroy the user supplied IV. You don't care about that?
> So I have to know that someone called setkey() on this ctx but I can't
> leave hints.

How about this? You extend the IV by one more byte, and use that
byte as a boolean flag to indicate whether the IV is valid. All
users that cannot supply their own IVs can then set the IV to zero.

When you see the zero flag in the IV, you reinitialise the IV per
the key.

> salsa also does not stick to plan here. ctx->input[6-9] is initialized
> in encrypt() path. So two threads sharing a ctx are going to clobber
> their state.

Salsa should also be fixed.

> What about a new api for the stream cipher? We would merge the ctx part
> and the iv into one handle. So the user would call setup_iv() instead of
> setkey(). The difference would be that I can access the iv from within
> setkey(). And the algorithm can fully express himself since he is no
> longer trapped in the wrong body

There is some merit in that, but as the current API can be made to do
the same thing (see above) I'm not convinced that this is worth the
cost for the moment.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Mon, Feb 22, 2010 at 08:45:47AM +0800, Herbert Xu wrote:
>
> How about this? You extend the IV by one more byte, and use that
> byte as a boolean flag to indicate whether the IV is valid. All
> users that cannot supply their own IVs can then set the IV to zero.
>
> When you see the zero flag in the IV, you reinitialise the IV per
> the key.

In fact for arc4 we could just drop the key altogether since it
plays no part after setting the initial state.

> > salsa also does not stick to plan here. ctx->input[6-9] is initialized
> > in encrypt() path. So two threads sharing a ctx are going to clobber
> > their state.
>
> Salsa should also be fixed.

For Salsa on the other hand the key is rather useful since all
we need is a two-byte IV that's just a sequence number.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Mon, Feb 22, 2010 at 02:40:49PM -0500, Mikulas Patocka wrote:
>
> > So what we could do is simply add a new blkcipher arc4, alongside
> > the existing cipher arc4. Then we can convert the existing users
> > across, and finally remove the old arc4.
>
> arc4 can't be used as a block cipher --- see this paper
> http://www.wisdom.weizmann.ac.il/~itsik/RC4/Papers/Rc4_ksa.ps , it says
> that initialization vectors on RC4 are unreliable, if you use (unknown key
> concatenated with known IV) or (known IV concatenated with unknown key) as
> a RC4 key, the RC4 state can be exposed and the cipher is broken.

What we call a blkcipher is not really a block cipher. In fact,
what we call "cipher" is really a block cipher. So we're actually
changing arc4 so that it doesn't get used as a block cipher, i.e.,
you will no longer be able to say "cbc(arc4)" or some such.

I know it's confusing and perhaps one day we will rename blkcipher
to skcipher and cipher to blkcipher.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Mon, Feb 22, 2010 at 11:08:35PM +0100, Sebastian Andrzej Siewior wrote:
> * Herbert Xu | 2010-02-22 08:52:17 [+0800]:
>
> >On Mon, Feb 22, 2010 at 08:45:47AM +0800, Herbert Xu wrote:
> >>
> >> How about this? You extend the IV by one more byte, and use that
> >> byte as a boolean flag to indicate whether the IV is valid. All
> So I trick the crypto api to allocate more bytes than ->ivsize says.

No tricks needed, just add the flag to the struct.

struct arc4_iv {
u8 S[256];
u8 x, y;
u8 valid;
};

> Okay. When we have to re-key and the user calls setkey() without
> re-allocating thr cipher then I would not notice this. So I need a
> counter. And all this will make it work but I still think it is fishy.
> Plus we waste 258bytes.

No you don't need to refresh the IV when the key changes. The
key should only be consulted when the valid flag in the IV is zero.

You need the 258 + flag bytes because that's just the amount of
state carried between the encrypt/decrypt operation. So it isn't
really wasted.

If you can find a way that allows arc4 to be used by multiple
threads at the same time while storing less than 258 bytes in
each thread, please let me know

> >In fact for arc4 we could just drop the key altogether since it
> >plays no part after setting the initial state.
> Since I'm not allowed to kfree() the ctx in encrypt() are you proposing
> tfm->setup_iv(iv, key)?

No, what you could do is structure the IV differently based on the
flag:

struct arc4_iv {
union {
struct key {
u8 key[256];
u16 keylen;
};
struct iv {
u8 S[256];
u8 x, y;
};
};
u8 type;
};

This relies on the fact that we never use more than 256 bytes in
the key so limiting its length is OK.

> >> Salsa should also be fixed.
> I saw that comming. And I complaind back then that the assembly code was
> not pretty enough... and removing the assembly is probably not option

Well if nobody steps in to fix the assembly then removing it is the
only option.

> >For Salsa on the other hand the key is rather useful since all
> >we need is a two-byte IV that's just a sequence number.
> No it's 8 bytes. Berstein's U8TO32_LITTLE() is actually a cpu_to_be32().
> Not sure if he knows it

Right.

> However I'm not sure where you going with this. salsa is fine besides
> the clobber thing, isn't it?

I don't know of any other problems.

Basically salsa should look pretty much like CTR from the outside
when it's fixed.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

On Sun, Mar 14, 2010 at 09:24:32AM +0100, Sebastian Andrzej Siewior wrote:
>
> Okay. So so are we talking about something like that below then? This is

Pretty much.

> untested and I break other users bexcept lib80211_crypt_tkip.

For the sake of compatibility please do this as a 3-step dance.
First add a new arc4 blkcipher, then convert the users, and
finally delete the old arc4.

> static int arc4_set_key(struct crypto_tfm *tfm, const u8 *in_key,
> unsigned int key_len)
> {
> - struct arc4_ctx *ctx = crypto_tfm_ctx(tfm);
> + /*
> + * ARC4 is special: The user should supply an IV as struct arc4_iv and
> + * fill either the key or the iv.
> + */
> + return -EOPNOTSUPP;
> +}

You should return 0 here. You should also set the min/max key
size to zero. The new arc4 no longer has a key as far as the
API is concerned.

Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel