2015-05-11 17:04:10

by Horia Geantă

[permalink] [raw]
Subject: [PATCH 1/4] crypto: talitos - avoid memleak in talitos_alg_alloc()

Cc: <[email protected]> # 3.2+
Fixes: 1d11911a8c57 ("crypto: talitos - fix warning: 'alg' may be used uninitialized in this function")
Signed-off-by: Horia Geanta <[email protected]>
---
drivers/crypto/talitos.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index c04074d08461..2c2ec0e0c145 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -2813,6 +2813,7 @@ static struct talitos_crypto_alg *talitos_alg_alloc(struct device *dev,
break;
default:
dev_err(dev, "unknown algorithm type %d\n", t_alg->algt.type);
+ kfree(t_alg);
return ERR_PTR(-EINVAL);
}

--
1.8.3.1


2015-05-11 17:18:54

by Horia Geantă

[permalink] [raw]
Subject: [PATCH 2/4] Revert "crypto: talitos - convert to use be16_add_cpu()"

This reverts commit 7291a932c6e27d9768e374e9d648086636daf61c.

The conversion to be16_add_cpu() is incorrect in case cryptlen is
negative due to premature (i.e. before addition / subtraction)
implicit conversion of cryptlen (int -> u16) leading to sign loss.

Cc: <[email protected]> # 3.10+
Cc: Wei Yongjun <[email protected]>
Signed-off-by: Horia Geanta <[email protected]>
---
drivers/crypto/talitos.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 2c2ec0e0c145..6b19b3dca616 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1083,7 +1083,8 @@ static int sg_to_link_tbl(struct scatterlist *sg, int sg_count,
sg_count--;
link_tbl_ptr--;
}
- be16_add_cpu(&link_tbl_ptr->len, cryptlen);
+ link_tbl_ptr->len = cpu_to_be16(be16_to_cpu(link_tbl_ptr->len)
+ + cryptlen);

/* tag end of link table */
link_tbl_ptr->j_extent = DESC_PTR_LNKTBL_RETURN;
--
1.8.3.1

2015-05-11 17:20:49

by Horia Geantă

[permalink] [raw]
Subject: [PATCH 4/4] crypto: talitos - static code checker fixes

-change req_ctx->nbuf from u64 to unsigned int to silence checker
warnings; this is safe since nbuf value is <= HASH_MAX_BLOCK_SIZE
-remove unused value read from TALITOS_CCPSR; there is no requirement
to read upper 32b before reading lower 32b of a 64b register;
SEC RM mentions: "reads can always be done by byte, word, or dword"
-remove unused return value of sg_to_link_tbl()
-change "len" parameter of map_single_talitos_ptr() and
to_talitos_ptr_len() to unsigned int; later, cpu_to_be16 will __force
downcast the value to unsigned short without any checker warning

Signed-off-by: Horia Geanta <[email protected]>
---
drivers/crypto/talitos.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 7367f6cba2ed..cb183b0b8578 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -63,7 +63,7 @@ static void to_talitos_ptr(struct talitos_ptr *ptr, dma_addr_t dma_addr,
ptr->eptr = upper_32_bits(dma_addr);
}

-static void to_talitos_ptr_len(struct talitos_ptr *ptr, unsigned short len,
+static void to_talitos_ptr_len(struct talitos_ptr *ptr, unsigned int len,
bool is_sec1)
{
if (is_sec1) {
@@ -94,7 +94,7 @@ static void to_talitos_ptr_extent_clear(struct talitos_ptr *ptr, bool is_sec1)
*/
static void map_single_talitos_ptr(struct device *dev,
struct talitos_ptr *ptr,
- unsigned short len, void *data,
+ unsigned int len, void *data,
enum dma_data_direction dir)
{
dma_addr_t dma_addr = dma_map_single(dev, data, len, dir);
@@ -543,7 +543,7 @@ static void talitos_error(struct device *dev, u32 isr, u32 isr_lo)
struct talitos_private *priv = dev_get_drvdata(dev);
unsigned int timeout = TALITOS_TIMEOUT;
int ch, error, reset_dev = 0;
- u32 v, v_lo;
+ u32 v_lo;
bool is_sec1 = has_ftr_sec1(priv);
int reset_ch = is_sec1 ? 1 : 0; /* only SEC2 supports continuation */

@@ -560,7 +560,6 @@ static void talitos_error(struct device *dev, u32 isr, u32 isr_lo)

error = -EINVAL;

- v = in_be32(priv->chan[ch].reg + TALITOS_CCPSR);
v_lo = in_be32(priv->chan[ch].reg + TALITOS_CCPSR_LO);

if (v_lo & TALITOS_CCPSR_LO_DOF) {
@@ -815,7 +814,7 @@ struct talitos_ahash_req_ctx {
unsigned int first;
unsigned int last;
unsigned int to_hash_later;
- u64 nbuf;
+ unsigned int nbuf;
struct scatterlist bufsl[2];
struct scatterlist *psrc;
};
@@ -1639,8 +1638,7 @@ void map_sg_out_talitos_ptr(struct device *dev, struct scatterlist *dst,
(edesc->src_nents + 1) *
sizeof(struct talitos_ptr), 0);
ptr->j_extent |= DESC_PTR_LNKTBL_JUMP;
- sg_count = sg_to_link_tbl(dst, sg_count, len,
- link_tbl_ptr);
+ sg_to_link_tbl(dst, sg_count, len, link_tbl_ptr);
dma_sync_single_for_device(dev, edesc->dma_link_tbl,
edesc->dma_len,
DMA_BIDIRECTIONAL);
--
1.8.3.1

2015-05-11 17:36:52

by Horia Geantă

[permalink] [raw]
Subject: [PATCH 2/4] Revert "crypto: talitos - convert to use be16_add_cpu()"

This reverts commit 7291a932c6e27d9768e374e9d648086636daf61c.

The conversion to be16_add_cpu() is incorrect in case cryptlen is
negative due to premature (i.e. before addition / subtraction)
implicit conversion of cryptlen (int -> u16) leading to sign loss.

Cc: <[email protected]> # 3.10+
Cc: Wei Yongjun <[email protected]>
Signed-off-by: Horia Geanta <[email protected]>
---
drivers/crypto/talitos.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 2c2ec0e0c145..6b19b3dca616 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1083,7 +1083,8 @@ static int sg_to_link_tbl(struct scatterlist *sg, int sg_count,
sg_count--;
link_tbl_ptr--;
}
- be16_add_cpu(&link_tbl_ptr->len, cryptlen);
+ link_tbl_ptr->len = cpu_to_be16(be16_to_cpu(link_tbl_ptr->len)
+ + cryptlen);

/* tag end of link table */
link_tbl_ptr->j_extent = DESC_PTR_LNKTBL_RETURN;
--
1.8.3.1

2015-05-12 08:28:15

by Horia Geantă

[permalink] [raw]
Subject: [PATCH 3/4] crypto: talitos - avoid out of bound scatterlist iterator

Check return value of scatterlist_sg_next(), i.e. don't rely solely
on number of bytes to be processed or number of scatterlist entries.

Signed-off-by: Horia Geanta <[email protected]>
---
drivers/crypto/talitos.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6b19b3dca616..7367f6cba2ed 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1065,7 +1065,7 @@ static int sg_to_link_tbl(struct scatterlist *sg, int sg_count,
{
int n_sg = sg_count;

- while (n_sg--) {
+ while (sg && n_sg--) {
to_talitos_ptr(link_tbl_ptr, sg_dma_address(sg), 0);
link_tbl_ptr->len = cpu_to_be16(sg_dma_len(sg));
link_tbl_ptr->j_extent = 0;
@@ -1254,7 +1254,7 @@ static int sg_count(struct scatterlist *sg_list, int nbytes, bool *chained)
int sg_nents = 0;

*chained = false;
- while (nbytes > 0) {
+ while (nbytes > 0 && sg) {
sg_nents++;
nbytes -= sg->length;
if (!sg_is_last(sg) && (sg + 1)->length == 0)
--
1.8.3.1

2015-05-13 03:20:12

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 1/4] crypto: talitos - avoid memleak in talitos_alg_alloc()

On Mon, May 11, 2015 at 08:03:24PM +0300, Horia Geanta wrote:
> Cc: <[email protected]> # 3.2+
> Fixes: 1d11911a8c57 ("crypto: talitos - fix warning: 'alg' may be used uninitialized in this function")
> Signed-off-by: Horia Geanta <[email protected]>

All applied.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt