2013-11-13 10:29:30

by Horia Geantă

Subject: [PATCH] crypto: talitos - fix locating offending descriptor in error path

Commit 3e721aeb3df3816e283ab18e327cd4652972e213
("crypto: talitos - handle descriptor not found in error path")
tried to address the fact that CDPR (Current Descriptor Pointer Register)
is unreliable.

As it turns out, there are still issues in the function detecting the
offending descriptor:
-only 32 bits of the descriptor address are read, however the address is
36-bit - since reset_channel() initializes channels with EAE (extended
address) bit set
-reading CDPR can return zero in cur_desc; when searching the channel
fifo for this address, cur_desc == dma_desc (= 0) case might happen,
leading to an oops when trying to return desc->hdr (desc is zero)
-read channel's .tail only once; the tail is a moving target; use a
local variable for the end of search condition

Signed-off-by: Lei Xu <[email protected]>
Signed-off-by: Horia Geanta <[email protected]>
Tested-by: Kalyani Chowdhury <[email protected]>
---
drivers/crypto/talitos.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index f6f7c68..04c5de3b 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -336,20 +336,29 @@ DEF_TALITOS_DONE(ch1_3, TALITOS_ISR_CH_1_3_DONE)
static u32 current_desc_hdr(struct device *dev, int ch)
{
struct talitos_private *priv = dev_get_drvdata(dev);
- int tail = priv->chan[ch].tail;
+ int tail, iter;
dma_addr_t cur_desc;

- cur_desc = in_be32(priv->chan[ch].reg + TALITOS_CDPR_LO);
+ cur_desc = ((u64)in_be32(priv->chan[ch].reg + TALITOS_CDPR)) << 32;
+ cur_desc |= in_be32(priv->chan[ch].reg + TALITOS_CDPR_LO);

- while (priv->chan[ch].fifo[tail].dma_desc != cur_desc) {
- tail = (tail + 1) & (priv->fifo_len - 1);
- if (tail == priv->chan[ch].tail) {
+ if (!cur_desc) {
+ dev_err(dev, "CDPR is NULL, giving up search for offending descriptor\n");
+ return 0;
+ }
+
+ tail = priv->chan[ch].tail;
+
+ iter = tail;
+ while (priv->chan[ch].fifo[iter].dma_desc != cur_desc) {
+ iter = (iter + 1) & (priv->fifo_len - 1);
+ if (iter == tail) {
dev_err(dev, "couldn't locate current descriptor\n");
return 0;
}
}

- return priv->chan[ch].fifo[tail].desc->hdr;
+ return priv->chan[ch].fifo[iter].desc->hdr;
}

/*
--
1.7.7.6

2013-12-05 14:35:47

by Herbert Xu

Subject: Re: [PATCH] crypto: talitos - fix locating offending descriptor in error path

On Wed, Nov 13, 2013 at 12:20:37PM +0200, Horia Geanta wrote:
> Commit 3e721aeb3df3816e283ab18e327cd4652972e213
> ("crypto: talitos - handle descriptor not found in error path")
> tried to address the fact that CDPR (Current Descriptor Pointer Register)
> is unreliable.
>
> As it turns out, there are still issues in the function detecting the
> offending descriptor:
> -only 32 bits of the descriptor address are read, however the address is
> 36-bit - since reset_channel() initializes channels with EAE (extended
> address) bit set
> -reading CDPR can return zero in cur_desc; when searching the channel
> fifo for this address, cur_desc == dma_desc (= 0) case might happen,
> leading to an oops when trying to return desc->hdr (desc is zero)
> -read channel's .tail only once; the tail is a moving target; use a
> local variable for the end of search condition
>
> Signed-off-by: Lei Xu <[email protected]>
> Signed-off-by: Horia Geanta <[email protected]>
> Tested-by: Kalyani Chowdhury <[email protected]>

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