2010-01-10 15:34:09

by Krzysztof Halasa

[permalink] [raw]
Subject: [6 PATCHes] IXP4xx crypto driver fixes.

Christian, Herbert,

I'm attaching 6 patches for IXP4xx crypto accelerator:

IXP4xx: Fix ixp4xx_crypto little-endian operation.
IXP4xx: Fix ixp4xx_crypto sparse warnings.
IXP4xx: Simplify get_crypt_desc() and get_crypt_desc_emerg() in ixp4xx_crypto.
IXP4xx: Fix whitespace problems in ixp4xx_crypto.
ixp4xx_crypto: Fix possible NULL ptr dereference.
ixp4xx_crypto: simplyfy the code a bit.

drivers/crypto/ixp4xx_crypto.c | 219 +++++++++++++++++++---------------------
1 files changed, 102 insertions(+), 117 deletions(-)


The first one fixes this on little-endian IXP425:

NPE-C: firmware functionality 0x5, revision 0x2:1
alg: skcipher: Test 1 failed on encryption for ecb(des)-ixp4xx
00000000: 01 23 45 67 89 ab cd e7
alg: skcipher: Test 1 failed on encryption for ecb(des3_ede)-ixp4xx
00000000: 73 6f 6d 65 64 61 74 61
alg: skcipher: Test 1 failed on encryption for ecb(aes)-ixp4xx
00000000: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff

There are missing tests there, not sure if they should be implemented
but it's a different story:

alg: No test for ...
authenc(hmac(md5),cbc(des))-ixp4xx
authenc(hmac(md5),cbc(des3_ede))-ixp4xx
authenc(hmac(sha1),cbc(des))-ixp4xx
authenc(hmac(sha1),cbc(des3_ede))-ixp4xx
authenc(hmac(md5),cbc(aes))-ixp4xx
authenc(hmac(sha1),cbc(aes))-ixp4xx

Not tested in any practical application yet, just got rid of the
warnings.

Since the changes are more related to IXP425 than they are to the crypto
code, I can send the patches to Linus myself, or you can do that, your
call.
Please state your ACK.
--
Krzysztof Halasa


2010-01-10 17:30:56

by Krzysztof Halasa

[permalink] [raw]
Subject: Re: [6 PATCHes] IXP4xx crypto driver fixes.

IXP4xx: Fix ixp4xx_crypto little-endian operation.

Signed-off-by: Krzysztof Hałasa <[email protected]>

Fixes the following on IXP425 little-endian:

NPE-C: firmware functionality 0x5, revision 0x2:1
alg: skcipher: Test 1 failed on encryption for ecb(des)-ixp4xx
00000000: 01 23 45 67 89 ab cd e7
alg: skcipher: Test 1 failed on encryption for ecb(des3_ede)-ixp4xx
00000000: 73 6f 6d 65 64 61 74 61
alg: skcipher: Test 1 failed on encryption for ecb(aes)-ixp4xx
00000000: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 6c6656d..cac026a 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -96,8 +96,13 @@

struct buffer_desc {
u32 phys_next;
+#ifdef __ARMEB__
u16 buf_len;
u16 pkt_len;
+#else
+ u16 pkt_len;
+ u16 buf_len;
+#endif
u32 phys_addr;
u32 __reserved[4];
struct buffer_desc *next;
@@ -105,17 +110,30 @@ struct buffer_desc {
};

struct crypt_ctl {
+#ifdef __ARMEB__
u8 mode; /* NPE_OP_* operation mode */
u8 init_len;
u16 reserved;
+#else
+ u16 reserved;
+ u8 init_len;
+ u8 mode; /* NPE_OP_* operation mode */
+#endif
u8 iv[MAX_IVLEN]; /* IV for CBC mode or CTR IV for CTR mode */
u32 icv_rev_aes; /* icv or rev aes */
u32 src_buf;
u32 dst_buf;
+#ifdef __ARMEB__
u16 auth_offs; /* Authentication start offset */
u16 auth_len; /* Authentication data length */
u16 crypt_offs; /* Cryption start offset */
u16 crypt_len; /* Cryption data length */
+#else
+ u16 auth_len; /* Authentication data length */
+ u16 auth_offs; /* Authentication start offset */
+ u16 crypt_len; /* Cryption data length */
+ u16 crypt_offs; /* Cryption start offset */
+#endif
u32 aadAddr; /* Additional Auth Data Addr for CCM mode */
u32 crypto_ctx; /* NPE Crypto Param structure address */

@@ -651,6 +669,9 @@ static int setup_auth(struct crypto_tfm *tfm, int encrypt, unsigned authsize,

/* write cfg word to cryptinfo */
cfgword = algo->cfgword | ( authsize << 6); /* (authsize/4) << 8 */
+#ifndef __ARMEB__
+ cfgword ^= 0xAA000000; /* change the "byte swap" flags */
+#endif
*(u32*)cinfo = cpu_to_be32(cfgword);
cinfo += sizeof(cfgword);

2010-01-10 17:32:16

by Krzysztof Halasa

[permalink] [raw]
Subject: IXP4xx: Fix ixp4xx_crypto sparse warnings.

Signed-off-by: Krzysztof Hałasa <[email protected]>

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index cac026a..99f06e1 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -402,7 +402,7 @@ static void one_packet(dma_addr_t phys)
break;
case CTL_FLAG_GEN_REVAES:
ctx = crypto_tfm_ctx(crypt->data.tfm);
- *(u32*)ctx->decrypt.npe_ctx &= cpu_to_be32(~CIPH_ENCR);
+ *(__be32 *)ctx->decrypt.npe_ctx &= cpu_to_be32(~CIPH_ENCR);
if (atomic_dec_and_test(&ctx->configuring))
complete(&ctx->completion);
break;
@@ -641,7 +641,7 @@ static int register_chain_var(struct crypto_tfm *tfm, u8 xpad, u32 target,
crypt->init_len = init_len;
crypt->ctl_flags |= CTL_FLAG_GEN_ICV;

- buf->next = 0;
+ buf->next = NULL;
buf->buf_len = HMAC_PAD_BLOCKLEN;
buf->pkt_len = 0;
buf->phys_addr = pad_phys;
@@ -672,7 +672,7 @@ static int setup_auth(struct crypto_tfm *tfm, int encrypt, unsigned authsize,
#ifndef __ARMEB__
cfgword ^= 0xAA000000; /* change the "byte swap" flags */
#endif
- *(u32*)cinfo = cpu_to_be32(cfgword);
+ *(__be32 *)cinfo = cpu_to_be32(cfgword);
cinfo += sizeof(cfgword);

/* write ICV to cryptinfo */
@@ -709,7 +709,7 @@ static int gen_rev_aes_key(struct crypto_tfm *tfm)
if (!crypt) {
return -EAGAIN;
}
- *(u32*)dir->npe_ctx |= cpu_to_be32(CIPH_ENCR);
+ *(__be32 *)dir->npe_ctx |= cpu_to_be32(CIPH_ENCR);

crypt->data.tfm = tfm;
crypt->crypt_offs = 0;
@@ -771,7 +771,7 @@ static int setup_cipher(struct crypto_tfm *tfm, int encrypt,
}
}
/* write cfg word to cryptinfo */
- *(u32*)cinfo = cpu_to_be32(cipher_cfg);
+ *(__be32 *)cinfo = cpu_to_be32(cipher_cfg);
cinfo += sizeof(cipher_cfg);

/* write cipher key to cryptinfo */

2010-01-10 17:32:55

by Krzysztof Halasa

[permalink] [raw]
Subject: IXP4xx: Simplify get_crypt_desc() and get_crypt_desc_emerg() in ixp4xx_crypto.

Signed-off-by: Krzysztof Hałasa <[email protected]>

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 99f06e1..0c7e4f5 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -277,26 +277,25 @@ static struct crypt_ctl *get_crypt_desc(void)
int i;
static int idx = 0;
unsigned long flags;
+ struct crypt_ctl *desc = NULL;

spin_lock_irqsave(&desc_lock, flags);

if (unlikely(!crypt_virt))
setup_crypt_desc();
- if (unlikely(!crypt_virt)) {
- spin_unlock_irqrestore(&desc_lock, flags);
- return NULL;
- }
+ if (unlikely(!crypt_virt))
+ goto out;
+
i = idx;
if (crypt_virt[i].ctl_flags == CTL_FLAG_UNUSED) {
if (++idx >= NPE_QLEN)
idx = 0;
crypt_virt[i].ctl_flags = CTL_FLAG_USED;
- spin_unlock_irqrestore(&desc_lock, flags);
- return crypt_virt +i;
- } else {
- spin_unlock_irqrestore(&desc_lock, flags);
- return NULL;
+ desc = crypt_virt + i;
}
+out:
+ spin_unlock_irqrestore(&desc_lock, flags);
+ return desc;
}

static spinlock_t emerg_lock;
@@ -319,12 +318,10 @@ static struct crypt_ctl *get_crypt_desc_emerg(void)
if (++idx >= NPE_QLEN_TOTAL)
idx = NPE_QLEN;
crypt_virt[i].ctl_flags = CTL_FLAG_USED;
- spin_unlock_irqrestore(&emerg_lock, flags);
- return crypt_virt +i;
- } else {
- spin_unlock_irqrestore(&emerg_lock, flags);
- return NULL;
+ desc = crypt_virt +i;
}
+ spin_unlock_irqrestore(&emerg_lock, flags);
+ return desc;
}

static void free_buf_chain(struct device *dev, struct buffer_desc *buf,u32 phys)

2010-01-10 17:33:41

by Krzysztof Halasa

[permalink] [raw]
Subject: IXP4xx: Fix whitespace problems in ixp4xx_crypto.

Signed-off-by: Krzysztof Hałasa <[email protected]>

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 0c7e4f5..f8f6515 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -64,7 +64,7 @@

#define MOD_DES 0x0000
#define MOD_TDEA2 0x0100
-#define MOD_3DES 0x0200
+#define MOD_3DES 0x0200
#define MOD_AES 0x0800
#define MOD_AES128 (0x0800 | KEYLEN_128)
#define MOD_AES192 (0x0900 | KEYLEN_192)
@@ -137,7 +137,7 @@ struct crypt_ctl {
u32 aadAddr; /* Additional Auth Data Addr for CCM mode */
u32 crypto_ctx; /* NPE Crypto Param structure address */

- /* Used by Host: 4*4 bytes*/
+ /* Used only by host: 4 * 4 bytes */
unsigned ctl_flags;
union {
struct ablkcipher_request *ablk_req;
@@ -208,10 +208,10 @@ static const struct ix_hash_algo hash_alg_sha1 = {
};

static struct npe *npe_c;
-static struct dma_pool *buffer_pool = NULL;
-static struct dma_pool *ctx_pool = NULL;
+static struct dma_pool *buffer_pool;
+static struct dma_pool *ctx_pool;

-static struct crypt_ctl *crypt_virt = NULL;
+static struct crypt_ctl *crypt_virt;
static dma_addr_t crypt_phys;

static int support_aes = 1;
@@ -246,12 +246,12 @@ static inline struct crypt_ctl *crypt_phys2virt(dma_addr_t phys)

static inline u32 cipher_cfg_enc(struct crypto_tfm *tfm)
{
- return container_of(tfm->__crt_alg, struct ixp_alg,crypto)->cfg_enc;
+ return container_of(tfm->__crt_alg, struct ixp_alg, crypto)->cfg_enc;
}

static inline u32 cipher_cfg_dec(struct crypto_tfm *tfm)
{
- return container_of(tfm->__crt_alg, struct ixp_alg,crypto)->cfg_dec;
+ return container_of(tfm->__crt_alg, struct ixp_alg, crypto)->cfg_dec;
}

static inline const struct ix_hash_algo *ix_hash(struct crypto_tfm *tfm)
@@ -275,7 +275,7 @@ static spinlock_t desc_lock;
static struct crypt_ctl *get_crypt_desc(void)
{
int i;
- static int idx = 0;
+ static int idx;
unsigned long flags;
struct crypt_ctl *desc = NULL;

@@ -318,13 +318,13 @@ static struct crypt_ctl *get_crypt_desc_emerg(void)
if (++idx >= NPE_QLEN_TOTAL)
idx = NPE_QLEN;
crypt_virt[i].ctl_flags = CTL_FLAG_USED;
- desc = crypt_virt +i;
+ desc = crypt_virt + i;
}
spin_unlock_irqrestore(&emerg_lock, flags);
return desc;
}

-static void free_buf_chain(struct device *dev, struct buffer_desc *buf,u32 phys)
+static void free_buf_chain(struct device *dev, struct buffer_desc *buf, u32 phys)
{
while (buf) {
struct buffer_desc *buf1;
@@ -349,10 +349,9 @@ static void finish_scattered_hmac(struct crypt_ctl *crypt)
int authsize = crypto_aead_authsize(tfm);
int decryptlen = req->cryptlen - authsize;

- if (req_ctx->encrypt) {
+ if (req_ctx->encrypt)
scatterwalk_map_and_copy(req_ctx->hmac_virt,
req->src, decryptlen, authsize, 1);
- }
dma_pool_free(buffer_pool, req_ctx->hmac_virt, crypt->icv_rev_aes);
}

@@ -372,9 +371,8 @@ static void one_packet(dma_addr_t phys)
struct aead_ctx *req_ctx = aead_request_ctx(req);

free_buf_chain(dev, req_ctx->buffer, crypt->src_buf);
- if (req_ctx->hmac_virt) {
+ if (req_ctx->hmac_virt)
finish_scattered_hmac(crypt);
- }
req->base.complete(&req->base, failed);
break;
}
@@ -382,9 +380,8 @@ static void one_packet(dma_addr_t phys)
struct ablkcipher_request *req = crypt->data.ablk_req;
struct ablk_ctx *req_ctx = ablkcipher_request_ctx(req);

- if (req_ctx->dst) {
+ if (req_ctx->dst)
free_buf_chain(dev, req_ctx->dst, crypt->dst_buf);
- }
free_buf_chain(dev, req_ctx->src, crypt->src_buf);
req->base.complete(&req->base, failed);
break;
@@ -418,7 +415,7 @@ static void crypto_done_action(unsigned long arg)
{
int i;

- for(i=0; i<4; i++) {
+ for (i = 0; i < 4; i++) {
dma_addr_t phys = qmgr_get_entry(RECV_QID);
if (!phys)
return;
@@ -443,9 +440,8 @@ static int init_ixp_crypto(void)

if (!npe_running(npe_c)) {
ret = npe_load_firmware(npe_c, npe_name(npe_c), dev);
- if (ret) {
+ if (ret)
return ret;
- }
if (npe_recv_message(npe_c, msg, "STATUS_MSG"))
goto npe_error;
} else {
@@ -478,14 +474,12 @@ static int init_ixp_crypto(void)
buffer_pool = dma_pool_create("buffer", dev,
sizeof(struct buffer_desc), 32, 0);
ret = -ENOMEM;
- if (!buffer_pool) {
+ if (!buffer_pool)
goto err;
- }
ctx_pool = dma_pool_create("context", dev,
NPE_CTX_LEN, 16, 0);
- if (!ctx_pool) {
+ if (!ctx_pool)
goto err;
- }
ret = qmgr_request_queue(SEND_QID, NPE_QLEN_TOTAL, 0, 0,
"ixp_crypto:out", NULL);
if (ret)
@@ -527,11 +521,10 @@ static void release_ixp_crypto(void)

npe_release(npe_c);

- if (crypt_virt) {
+ if (crypt_virt)
dma_free_coherent(dev,
- NPE_QLEN_TOTAL * sizeof( struct crypt_ctl),
+ NPE_QLEN_TOTAL * sizeof(struct crypt_ctl),
crypt_virt, crypt_phys);
- }
return;
}

@@ -545,9 +538,8 @@ static void reset_sa_dir(struct ix_sa_dir *dir)
static int init_sa_dir(struct ix_sa_dir *dir)
{
dir->npe_ctx = dma_pool_alloc(ctx_pool, GFP_KERNEL, &dir->npe_ctx_phys);
- if (!dir->npe_ctx) {
+ if (!dir->npe_ctx)
return -ENOMEM;
- }
reset_sa_dir(dir);
return 0;
}
@@ -568,9 +560,8 @@ static int init_tfm(struct crypto_tfm *tfm)
if (ret)
return ret;
ret = init_sa_dir(&ctx->decrypt);
- if (ret) {
+ if (ret)
free_sa_dir(&ctx->encrypt);
- }
return ret;
}

@@ -621,9 +612,8 @@ static int register_chain_var(struct crypto_tfm *tfm, u8 xpad, u32 target,

memcpy(pad, key, key_len);
memset(pad + key_len, 0, HMAC_PAD_BLOCKLEN - key_len);
- for (i = 0; i < HMAC_PAD_BLOCKLEN; i++) {
+ for (i = 0; i < HMAC_PAD_BLOCKLEN; i++)
pad[i] ^= xpad;
- }

crypt->data.tfm = tfm;
crypt->regist_ptr = pad;
@@ -665,7 +655,7 @@ static int setup_auth(struct crypto_tfm *tfm, int encrypt, unsigned authsize,
algo = ix_hash(tfm);

/* write cfg word to cryptinfo */
- cfgword = algo->cfgword | ( authsize << 6); /* (authsize/4) << 8 */
+ cfgword = algo->cfgword | (authsize << 6); /* (authsize/4) << 8 */
#ifndef __ARMEB__
cfgword ^= 0xAA000000; /* change the "byte swap" flags */
#endif
@@ -703,9 +693,8 @@ static int gen_rev_aes_key(struct crypto_tfm *tfm)
struct ix_sa_dir *dir = &ctx->decrypt;

crypt = get_crypt_desc_emerg();
- if (!crypt) {
+ if (!crypt)
return -EAGAIN;
- }
*(__be32 *)dir->npe_ctx |= cpu_to_be32(CIPH_ENCR);

crypt->data.tfm = tfm;
@@ -740,32 +729,30 @@ static int setup_cipher(struct crypto_tfm *tfm, int encrypt,
if (encrypt) {
cipher_cfg = cipher_cfg_enc(tfm);
dir->npe_mode |= NPE_OP_CRYPT_ENCRYPT;
- } else {
+ } else
cipher_cfg = cipher_cfg_dec(tfm);
- }
+
if (cipher_cfg & MOD_AES) {
switch (key_len) {
- case 16: keylen_cfg = MOD_AES128 | KEYLEN_128; break;
- case 24: keylen_cfg = MOD_AES192 | KEYLEN_192; break;
- case 32: keylen_cfg = MOD_AES256 | KEYLEN_256; break;
- default:
- *flags |= CRYPTO_TFM_RES_BAD_KEY_LEN;
- return -EINVAL;
+ case 16: keylen_cfg = MOD_AES128 | KEYLEN_128; break;
+ case 24: keylen_cfg = MOD_AES192 | KEYLEN_192; break;
+ case 32: keylen_cfg = MOD_AES256 | KEYLEN_256; break;
+ default:
+ *flags |= CRYPTO_TFM_RES_BAD_KEY_LEN;
+ return -EINVAL;
}
cipher_cfg |= keylen_cfg;
} else if (cipher_cfg & MOD_3DES) {
const u32 *K = (const u32 *)key;
if (unlikely(!((K[0] ^ K[2]) | (K[1] ^ K[3])) ||
- !((K[2] ^ K[4]) | (K[3] ^ K[5]))))
- {
+ !((K[2] ^ K[4]) | (K[3] ^ K[5])))) {
*flags |= CRYPTO_TFM_RES_BAD_KEY_SCHED;
return -EINVAL;
}
} else {
u32 tmp[DES_EXPKEY_WORDS];
- if (des_ekey(tmp, key) == 0) {
+ if (des_ekey(tmp, key) == 0)
*flags |= CRYPTO_TFM_RES_WEAK_KEY;
- }
}
/* write cfg word to cryptinfo */
*(__be32 *)cinfo = cpu_to_be32(cipher_cfg);
@@ -775,14 +762,13 @@ static int setup_cipher(struct crypto_tfm *tfm, int encrypt,
memcpy(cinfo, key, key_len);
/* NPE wants keylen set to DES3_EDE_KEY_SIZE even for single DES */
if (key_len < DES3_EDE_KEY_SIZE && !(cipher_cfg & MOD_AES)) {
- memset(cinfo + key_len, 0, DES3_EDE_KEY_SIZE -key_len);
+ memset(cinfo + key_len, 0, DES3_EDE_KEY_SIZE - key_len);
key_len = DES3_EDE_KEY_SIZE;
}
dir->npe_ctx_idx = sizeof(cipher_cfg) + key_len;
dir->npe_mode |= NPE_OP_CRYPT_ENABLE;
- if ((cipher_cfg & MOD_AES) && !encrypt) {
+ if ((cipher_cfg & MOD_AES) && !encrypt)
return gen_rev_aes_key(tfm);
- }
return 0;
}

@@ -791,7 +777,7 @@ static struct buffer_desc *chainup_buffers(struct device *dev,
struct buffer_desc *buf, gfp_t flags,
enum dma_data_direction dir)
{
- for (;nbytes > 0; sg = scatterwalk_sg_next(sg)) {
+ for (; nbytes > 0; sg = scatterwalk_sg_next(sg)) {
unsigned len = min(nbytes, sg->length);
struct buffer_desc *next_buf;
u32 next_buf_phys;
@@ -842,11 +828,10 @@ static int ablk_setkey(struct crypto_ablkcipher *tfm, const u8 *key,
goto out;

if (*flags & CRYPTO_TFM_RES_WEAK_KEY) {
- if (*flags & CRYPTO_TFM_REQ_WEAK_KEY) {
+ if (*flags & CRYPTO_TFM_REQ_WEAK_KEY)
ret = -EINVAL;
- } else {
+ else
*flags &= ~CRYPTO_TFM_RES_WEAK_KEY;
- }
}
out:
if (!atomic_dec_and_test(&ctx->configuring))
@@ -918,9 +903,8 @@ static int ablk_perform(struct ablkcipher_request *req, int encrypt)
src_direction = DMA_TO_DEVICE;
req_ctx->dst = dst_hook.next;
crypt->dst_buf = dst_hook.phys_next;
- } else {
+ } else
req_ctx->dst = NULL;
- }
req_ctx->src = NULL;
if (!chainup_buffers(dev, req->src, nbytes, &src_hook,
flags, src_direction))
@@ -936,9 +920,8 @@ static int ablk_perform(struct ablkcipher_request *req, int encrypt)
free_buf_src:
free_buf_chain(dev, req_ctx->src, crypt->src_buf);
free_buf_dest:
- if (req->src != req->dst) {
+ if (req->src != req->dst)
free_buf_chain(dev, req_ctx->dst, crypt->dst_buf);
- }
crypt->ctl_flags = CTL_FLAG_UNUSED;
return -ENOMEM;
}
@@ -962,7 +945,7 @@ static int ablk_rfc3686_crypt(struct ablkcipher_request *req)
int ret;

/* set up counter block */
- memcpy(iv, ctx->nonce, CTR_RFC3686_NONCE_SIZE);
+ memcpy(iv, ctx->nonce, CTR_RFC3686_NONCE_SIZE);
memcpy(iv + CTR_RFC3686_NONCE_SIZE, info, CTR_RFC3686_IV_SIZE);

/* initialize counter portion of counter block */
@@ -1019,7 +1002,7 @@ static int aead_perform(struct aead_request *req, int encrypt,
} else {
dir = &ctx->decrypt;
/* req->cryptlen includes the authsize when decrypting */
- cryptlen = req->cryptlen -authsize;
+ cryptlen = req->cryptlen - authsize;
eff_cryptlen -= authsize;
}
crypt = get_crypt_desc();
@@ -1039,9 +1022,8 @@ static int aead_perform(struct aead_request *req, int encrypt,
BUG_ON(ivsize && !req->iv);
memcpy(crypt->iv, req->iv, ivsize);

- if (req->src != req->dst) {
+ if (req->src != req->dst)
BUG(); /* -ENOTSUP because of my lazyness */
- }

/* ASSOC data */
buf = chainup_buffers(dev, req->assoc, req->assoclen, &src_hook,
@@ -1064,32 +1046,28 @@ static int aead_perform(struct aead_request *req, int encrypt,
&crypt->icv_rev_aes);
if (unlikely(!req_ctx->hmac_virt))
goto free_chain;
- if (!encrypt) {
+ if (!encrypt)
scatterwalk_map_and_copy(req_ctx->hmac_virt,
req->src, cryptlen, authsize, 0);
- }
req_ctx->encrypt = encrypt;
- } else {
+ } else
req_ctx->hmac_virt = NULL;
- }
/* Crypt */
buf = chainup_buffers(dev, req->src, cryptlen + authsize, buf, flags,
DMA_BIDIRECTIONAL);
if (!buf)
goto free_hmac_virt;
- if (!req_ctx->hmac_virt) {
+ if (!req_ctx->hmac_virt)
crypt->icv_rev_aes = buf->phys_addr + buf->buf_len - authsize;
- }

crypt->ctl_flags |= CTL_FLAG_PERFORM_AEAD;
qmgr_put_entry(SEND_QID, crypt_virt2phys(crypt));
BUG_ON(qmgr_stat_overflow(SEND_QID));
return -EINPROGRESS;
free_hmac_virt:
- if (req_ctx->hmac_virt) {
+ if (req_ctx->hmac_virt)
dma_pool_free(buffer_pool, req_ctx->hmac_virt,
crypt->icv_rev_aes);
- }
free_chain:
free_buf_chain(dev, req_ctx->buffer, crypt->src_buf);
out:
@@ -1131,9 +1109,8 @@ static int aead_setup(struct crypto_aead *tfm, unsigned int authsize)
if (*flags & CRYPTO_TFM_REQ_WEAK_KEY) {
ret = -EINVAL;
goto out;
- } else {
+ } else
*flags &= ~CRYPTO_TFM_RES_WEAK_KEY;
- }
}
out:
if (!atomic_dec_and_test(&ctx->configuring))
@@ -1219,7 +1196,7 @@ static int aead_givencrypt(struct aead_givcrypt_request *req)
seq = cpu_to_be64(req->seq);
memcpy(req->giv + ivsize - len, &seq, len);
return aead_perform(&req->areq, 1, req->areq.assoclen,
- req->areq.cryptlen +ivsize, req->giv);
+ req->areq.cryptlen + ivsize, req->giv);
}

static struct ixp_alg ixp4xx_algos[] = {
@@ -1416,7 +1393,7 @@ static struct ixp_alg ixp4xx_algos[] = {
static int __init ixp_module_init(void)
{
int num = ARRAY_SIZE(ixp4xx_algos);
- int i,err ;
+ int i, err ;

if (platform_device_register(&pseudo_dev))
return -ENODEV;
@@ -1429,18 +1406,14 @@ static int __init ixp_module_init(void)
platform_device_unregister(&pseudo_dev);
return err;
}
- for (i=0; i< num; i++) {
+ for (i = 0; i < num; i++) {
struct crypto_alg *cra = &ixp4xx_algos[i].crypto;

if (snprintf(cra->cra_driver_name, CRYPTO_MAX_ALG_NAME,
- "%s"IXP_POSTFIX, cra->cra_name) >=
- CRYPTO_MAX_ALG_NAME)
- {
+ "%s"IXP_POSTFIX, cra->cra_name) >= CRYPTO_MAX_ALG_NAME)
continue;
- }
- if (!support_aes && (ixp4xx_algos[i].cfg_enc & MOD_AES)) {
+ if (!support_aes && (ixp4xx_algos[i].cfg_enc & MOD_AES))
continue;
- }
if (!ixp4xx_algos[i].hash) {
/* block ciphers */
cra->cra_type = &crypto_ablkcipher_type;
@@ -1484,7 +1457,7 @@ static void __exit ixp_module_exit(void)
int num = ARRAY_SIZE(ixp4xx_algos);
int i;

- for (i=0; i< num; i++) {
+ for (i = 0; i < num; i++) {
if (ixp4xx_algos[i].registered)
crypto_unregister_alg(&ixp4xx_algos[i].crypto);
}

2010-01-10 17:37:59

by Krzysztof Halasa

[permalink] [raw]
Subject: ixp4xx_crypto: simplyfy the code a bit.

Signed-off-by: Krzysztof Hałasa <[email protected]>

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 2ae7148..a28df93 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -104,7 +104,7 @@ struct buffer_desc {
u16 buf_len;
#endif
u32 phys_addr;
- u32 __reserved[4];
+ u32 __reserved[5];
struct buffer_desc *next;
enum dma_data_direction dir;
};
@@ -120,7 +120,7 @@ struct crypt_ctl {
u8 mode; /* NPE_OP_* operation mode */
#endif
u8 iv[MAX_IVLEN]; /* IV for CBC mode or CTR IV for CTR mode */
- u32 icv_rev_aes; /* icv or rev aes */
+ u32 icv_rev_aes; /* address to store icv or rev aes */
u32 src_buf;
u32 dst_buf;
#ifdef __ARMEB__
@@ -429,8 +429,8 @@ static int init_ixp_crypto(void)
int ret = -ENODEV;
u32 msg[2] = { 0, 0 };

- if (! ( ~(*IXP4XX_EXP_CFG2) & (IXP4XX_FEATURE_HASH |
- IXP4XX_FEATURE_AES | IXP4XX_FEATURE_DES))) {
+ if (!(ixp4xx_read_feature_bits() &
+ (IXP4XX_FEATURE_HASH | IXP4XX_FEATURE_AES | IXP4XX_FEATURE_DES))) {
printk(KERN_ERR "ixp_crypto: No HW crypto available\n");
return ret;
}
@@ -442,15 +442,11 @@ static int init_ixp_crypto(void)
ret = npe_load_firmware(npe_c, npe_name(npe_c), dev);
if (ret)
return ret;
- if (npe_recv_message(npe_c, msg, "STATUS_MSG"))
- goto npe_error;
- } else {
- if (npe_send_message(npe_c, msg, "STATUS_MSG"))
- goto npe_error;
+ } else if (npe_send_message(npe_c, msg, "STATUS_MSG"))
+ goto npe_error;

- if (npe_recv_message(npe_c, msg, "STATUS_MSG"))
- goto npe_error;
- }
+ if (npe_recv_message(npe_c, msg, "STATUS_MSG"))
+ goto npe_error;

switch ((msg[1]>>16) & 0xff) {
case 3:
@@ -1120,7 +1116,7 @@ static int aead_setauthsize(struct crypto_aead *tfm, unsigned int authsize)
{
int max = crypto_aead_alg(tfm)->maxauthsize >> 2;

- if ((authsize>>2) < 1 || (authsize>>2) > max || (authsize & 3))
+ if ((authsize >> 2) < 1 || (authsize >> 2) > max || (authsize & 3))
return -EINVAL;
return aead_setup(tfm, authsize);
}

2010-01-10 17:37:28

by Krzysztof Halasa

[permalink] [raw]
Subject: ixp4xx_crypto: Fix possible NULL ptr dereference.

Signed-off-by: Krzysztof Hałasa <[email protected]>

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index f8f6515..2ae7148 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -786,10 +786,8 @@ static struct buffer_desc *chainup_buffers(struct device *dev,
nbytes -= len;
ptr = page_address(sg_page(sg)) + sg->offset;
next_buf = dma_pool_alloc(buffer_pool, flags, &next_buf_phys);
- if (!next_buf) {
- buf = NULL;
- break;
- }
+ if (!next_buf)
+ return NULL;
sg_dma_address(sg) = dma_map_single(dev, ptr, len, dir);
buf->next = next_buf;
buf->phys_next = next_buf_phys;

2010-01-11 09:08:38

by Christian Hohnstaedt

[permalink] [raw]
Subject: Re: [6 PATCHes] IXP4xx crypto driver fixes.

On Sun, Jan 10, 2010 at 06:30:53PM +0100, Krzysztof Halasa wrote:
> IXP4xx: Fix ixp4xx_crypto little-endian operation.
>
> Signed-off-by: Krzysztof Hałasa <[email protected]>
Acked-by: Christian Hohnstaedt <[email protected]>

>
> Fixes the following on IXP425 little-endian:
>
> NPE-C: firmware functionality 0x5, revision 0x2:1
> alg: skcipher: Test 1 failed on encryption for ecb(des)-ixp4xx
> 00000000: 01 23 45 67 89 ab cd e7
> alg: skcipher: Test 1 failed on encryption for ecb(des3_ede)-ixp4xx
> 00000000: 73 6f 6d 65 64 61 74 61
> alg: skcipher: Test 1 failed on encryption for ecb(aes)-ixp4xx
> 00000000: 00 11 22 33 44 55 66 77 88 99 aa bb cc dd ee ff
>
> diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
> index 6c6656d..cac026a 100644
> --- a/drivers/crypto/ixp4xx_crypto.c
> +++ b/drivers/crypto/ixp4xx_crypto.c
> @@ -96,8 +96,13 @@
>
> struct buffer_desc {
> u32 phys_next;
> +#ifdef __ARMEB__
> u16 buf_len;
> u16 pkt_len;
> +#else
> + u16 pkt_len;
> + u16 buf_len;
> +#endif
> u32 phys_addr;
> u32 __reserved[4];
> struct buffer_desc *next;
> @@ -105,17 +110,30 @@ struct buffer_desc {
> };
>
> struct crypt_ctl {
> +#ifdef __ARMEB__
> u8 mode; /* NPE_OP_* operation mode */
> u8 init_len;
> u16 reserved;
> +#else
> + u16 reserved;
> + u8 init_len;
> + u8 mode; /* NPE_OP_* operation mode */
> +#endif
> u8 iv[MAX_IVLEN]; /* IV for CBC mode or CTR IV for CTR mode */
> u32 icv_rev_aes; /* icv or rev aes */
> u32 src_buf;
> u32 dst_buf;
> +#ifdef __ARMEB__
> u16 auth_offs; /* Authentication start offset */
> u16 auth_len; /* Authentication data length */
> u16 crypt_offs; /* Cryption start offset */
> u16 crypt_len; /* Cryption data length */
> +#else
> + u16 auth_len; /* Authentication data length */
> + u16 auth_offs; /* Authentication start offset */
> + u16 crypt_len; /* Cryption data length */
> + u16 crypt_offs; /* Cryption start offset */
> +#endif
> u32 aadAddr; /* Additional Auth Data Addr for CCM mode */
> u32 crypto_ctx; /* NPE Crypto Param structure address */
>
> @@ -651,6 +669,9 @@ static int setup_auth(struct crypto_tfm *tfm, int encrypt, unsigned authsize,
>
> /* write cfg word to cryptinfo */
> cfgword = algo->cfgword | ( authsize << 6); /* (authsize/4) << 8 */
> +#ifndef __ARMEB__
> + cfgword ^= 0xAA000000; /* change the "byte swap" flags */
> +#endif
> *(u32*)cinfo = cpu_to_be32(cfgword);
> cinfo += sizeof(cfgword);
>
Christian Hohnstaedt

--
Christian Hohnstaedt / Project Manager Hardware and Manufacturing

Innominate Security Technologies AG / protecting industrial networks
tel: +49.30.921028.208 / fax: +49.30.921028.020
Rudower Chaussee 13, D-12489 Berlin / http://www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Volker Bibelhausen

2010-01-11 10:07:13

by Christian Hohnstaedt

[permalink] [raw]
Subject: Re: ixp4xx_crypto: Fix possible NULL ptr dereference.

On Sun, Jan 10, 2010 at 06:37:25PM +0100, Krzysztof Halasa wrote:
> Signed-off-by: Krzysztof Hałasa <[email protected]>
>
> diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
> index f8f6515..2ae7148 100644
> --- a/drivers/crypto/ixp4xx_crypto.c
> +++ b/drivers/crypto/ixp4xx_crypto.c
> @@ -786,10 +786,8 @@ static struct buffer_desc *chainup_buffers(struct device *dev,
> nbytes -= len;
> ptr = page_address(sg_page(sg)) + sg->offset;
> next_buf = dma_pool_alloc(buffer_pool, flags, &next_buf_phys);
> - if (!next_buf) {
> - buf = NULL;
> - break;
> - }
> + if (!next_buf)
> + return NULL;

This leaves buf->next uninitialized, but
free_buf_chain() iterates over buf->next.

We need:

if (!next_buf) {
buf->next = NULL;
return NULL;
}

Or get rid of next_buf and next_buf_phys:

diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index b8cc714..c961b0f 100644
--- a/drivers/crypto/ixp4xx_crypto.c
+++ b/drivers/crypto/ixp4xx_crypto.c
@@ -794,21 +794,15 @@ static struct buffer_desc *chainup_buffers(struct device *dev,
{
for (;nbytes > 0; sg = scatterwalk_sg_next(sg)) {
unsigned len = min(nbytes, sg->length);
- struct buffer_desc *next_buf;
- u32 next_buf_phys;
void *ptr;

nbytes -= len;
ptr = page_address(sg_page(sg)) + sg->offset;
- next_buf = dma_pool_alloc(buffer_pool, flags, &next_buf_phys);
- if (!next_buf) {
- buf = NULL;
- break;
- }
+ buf->next = dma_pool_alloc(buffer_pool, flags, &buf->phys_next);
+ if (!buf->next)
+ return NULL;
sg_dma_address(sg) = dma_map_single(dev, ptr, len, dir);
- buf->next = next_buf;
- buf->phys_next = next_buf_phys;
- buf = next_buf;
+ buf = buf->next;

buf->phys_addr = sg_dma_address(sg);
buf->buf_len = len;


Christian Hohnstaedt

--
Christian Hohnstaedt / Project Manager Hardware and Manufacturing

Innominate Security Technologies AG / protecting industrial networks
tel: +49.30.921028.208 / fax: +49.30.921028.020
Rudower Chaussee 13, D-12489 Berlin / http://www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Volker Bibelhausen

2010-01-12 17:12:03

by Christian Hohnstaedt

[permalink] [raw]
Subject: Re: IXP4xx: Fix whitespace problems in ixp4xx_crypto.

On Sun, Jan 10, 2010 at 06:33:37PM +0100, Krzysztof Halasa wrote:
> Signed-off-by: Krzysztof Hałasa <[email protected]>
>
> diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
> index 0c7e4f5..f8f6515 100644
> --- a/drivers/crypto/ixp4xx_crypto.c
> +++ b/drivers/crypto/ixp4xx_crypto.c
> @@ -64,7 +64,7 @@
>
> #define MOD_DES 0x0000
> #define MOD_TDEA2 0x0100
> -#define MOD_3DES 0x0200
> +#define MOD_3DES 0x0200
> #define MOD_AES 0x0800
> #define MOD_AES128 (0x0800 | KEYLEN_128)
> #define MOD_AES192 (0x0900 | KEYLEN_192)
> @@ -137,7 +137,7 @@ struct crypt_ctl {
> u32 aadAddr; /* Additional Auth Data Addr for CCM mode */
> u32 crypto_ctx; /* NPE Crypto Param structure address */
>
> - /* Used by Host: 4*4 bytes*/
> + /* Used only by host: 4 * 4 bytes */
> unsigned ctl_flags;
> union {
> struct ablkcipher_request *ablk_req;
> @@ -208,10 +208,10 @@ static const struct ix_hash_algo hash_alg_sha1 = {
> };
>
> static struct npe *npe_c;
> -static struct dma_pool *buffer_pool = NULL;
> -static struct dma_pool *ctx_pool = NULL;
> +static struct dma_pool *buffer_pool;
> +static struct dma_pool *ctx_pool;
>
> -static struct crypt_ctl *crypt_virt = NULL;
> +static struct crypt_ctl *crypt_virt;

This is not a whitespace-fix.
The error-path in init_ixp_crypto() depends on them being either NULL
or correctly allocated.

Or is it guaranteed that static variables are always initially zero ?

> static dma_addr_t crypt_phys;
>
> static int support_aes = 1;

But this initialization is superflous, since it will be initialized before use.

> @@ -246,12 +246,12 @@ static inline struct crypt_ctl *crypt_phys2virt(dma_addr_t phys)
>
> static inline u32 cipher_cfg_enc(struct crypto_tfm *tfm)
> {
> - return container_of(tfm->__crt_alg, struct ixp_alg,crypto)->cfg_enc;
> + return container_of(tfm->__crt_alg, struct ixp_alg, crypto)->cfg_enc;
> }
>
> static inline u32 cipher_cfg_dec(struct crypto_tfm *tfm)
> {
> - return container_of(tfm->__crt_alg, struct ixp_alg,crypto)->cfg_dec;
> + return container_of(tfm->__crt_alg, struct ixp_alg, crypto)->cfg_dec;
> }
>
> static inline const struct ix_hash_algo *ix_hash(struct crypto_tfm *tfm)
> @@ -275,7 +275,7 @@ static spinlock_t desc_lock;
> static struct crypt_ctl *get_crypt_desc(void)
> {
> int i;
> - static int idx = 0;
> + static int idx;

This static index must be initialized with 0.

> unsigned long flags;
> struct crypt_ctl *desc = NULL;
>
> @@ -318,13 +318,13 @@ static struct crypt_ctl *get_crypt_desc_emerg(void)
> if (++idx >= NPE_QLEN_TOTAL)
> idx = NPE_QLEN;
> crypt_virt[i].ctl_flags = CTL_FLAG_USED;
> - desc = crypt_virt +i;
> + desc = crypt_virt + i;
> }
> spin_unlock_irqrestore(&emerg_lock, flags);
> return desc;
> }
>
> -static void free_buf_chain(struct device *dev, struct buffer_desc *buf,u32 phys)
> +static void free_buf_chain(struct device *dev, struct buffer_desc *buf, u32 phys)

Introduces a line-length > 80.

> {
> while (buf) {
> struct buffer_desc *buf1;
> @@ -349,10 +349,9 @@ static void finish_scattered_hmac(struct crypt_ctl *crypt)
> int authsize = crypto_aead_authsize(tfm);

[ snip ]

> @@ -1416,7 +1393,7 @@ static struct ixp_alg ixp4xx_algos[] = {
> static int __init ixp_module_init(void)
> {
> int num = ARRAY_SIZE(ixp4xx_algos);
> - int i,err ;
> + int i, err ;

Missed one before the ;

>
> if (platform_device_register(&pseudo_dev))
> return -ENODEV;
> @@ -1429,18 +1406,14 @@ static int __init ixp_module_init(void)
> platform_device_unregister(&pseudo_dev);
> return err;
> }
> - for (i=0; i< num; i++) {
> + for (i = 0; i < num; i++) {
> struct crypto_alg *cra = &ixp4xx_algos[i].crypto;
>
> if (snprintf(cra->cra_driver_name, CRYPTO_MAX_ALG_NAME,
> - "%s"IXP_POSTFIX, cra->cra_name) >=
> - CRYPTO_MAX_ALG_NAME)
> - {
> + "%s"IXP_POSTFIX, cra->cra_name) >= CRYPTO_MAX_ALG_NAME)
> continue;
> - }
> - if (!support_aes && (ixp4xx_algos[i].cfg_enc & MOD_AES)) {
> + if (!support_aes && (ixp4xx_algos[i].cfg_enc & MOD_AES))
> continue;
> - }
> if (!ixp4xx_algos[i].hash) {
> /* block ciphers */
> cra->cra_type = &crypto_ablkcipher_type;
> @@ -1484,7 +1457,7 @@ static void __exit ixp_module_exit(void)
> int num = ARRAY_SIZE(ixp4xx_algos);
> int i;
>
> - for (i=0; i< num; i++) {
> + for (i = 0; i < num; i++) {
> if (ixp4xx_algos[i].registered)
> crypto_unregister_alg(&ixp4xx_algos[i].crypto);
> }

Christian Hohnstaedt

--
Christian Hohnstaedt / Project Manager Hardware and Manufacturing

Innominate Security Technologies AG / protecting industrial networks
tel: +49.30.921028.208 / fax: +49.30.921028.020
Rudower Chaussee 13, D-12489 Berlin / http://www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Volker Bibelhausen

2010-01-12 18:09:22

by Krzysztof Halasa

[permalink] [raw]
Subject: Re: IXP4xx: Fix whitespace problems in ixp4xx_crypto.

Christian Hohnstaedt <[email protected]> writes:

>> static struct npe *npe_c;
>> -static struct dma_pool *buffer_pool = NULL;
>> -static struct dma_pool *ctx_pool = NULL;
>> +static struct dma_pool *buffer_pool;
>> +static struct dma_pool *ctx_pool;
>>
>> -static struct crypt_ctl *crypt_virt = NULL;
>> +static struct crypt_ctl *crypt_virt;
>
> This is not a whitespace-fix.

Right, that's trivial non-whitespace fix :-)

> The error-path in init_ixp_crypto() depends on them being either NULL
> or correctly allocated.
>
> Or is it guaranteed that static variables are always initially zero ?

Yes, the BSS is cleared at boot (modprobe etc). This simply makes the
on-disk image a bit smaller.

>> static dma_addr_t crypt_phys;
>>
>> static int support_aes = 1;
>
> But this initialization is superflous, since it will be initialized
> before use.

I didn't touch it, but will remove the initialization if it's unneeded,
of course.

>> - static int idx = 0;
>> + static int idx;
>
> This static index must be initialized with 0.

It is, same as the crypt_virt and co.

>> -static void free_buf_chain(struct device *dev, struct buffer_desc *buf,u32 phys)
>> +static void free_buf_chain(struct device *dev, struct buffer_desc *buf, u32 phys)
>
> Introduces a line-length > 80.

This limit has been lifted recently :-)

>> int num = ARRAY_SIZE(ixp4xx_algos);
>> - int i,err ;
>> + int i, err ;
>
> Missed one before the ;

Right.


I will fix/change these, not today but soon. Thanks for looking.
--
Krzysztof Halasa