The error handling in stm32_hash_irq_thread passes
uninitialized data into stm32_hash_finish_req, as gcc
points out:
drivers/crypto/stm32/stm32-hash.c: In function 'stm32_hash_irq_thread':
drivers/crypto/stm32/stm32-hash.c:1088:2: error: 'err' may be used uninitialized in this function [-Werror=maybe-uninitialized]
I could not tell what data should be passed there instead,
so this changes the code to always pass zero, making it
well-defined, though possibly still wrong. Please check.
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/crypto/stm32/stm32-hash.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index b585ce54a802..3c23a23e9ee5 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -1067,7 +1067,6 @@ static int stm32_hash_cra_sha256_init(struct crypto_tfm *tfm)
static irqreturn_t stm32_hash_irq_thread(int irq, void *dev_id)
{
struct stm32_hash_dev *hdev = dev_id;
- int err;
if (HASH_FLAGS_CPU & hdev->flags) {
if (HASH_FLAGS_OUTPUT_READY & hdev->flags) {
@@ -1085,7 +1084,7 @@ static irqreturn_t stm32_hash_irq_thread(int irq, void *dev_id)
finish:
/*Finish current request */
- stm32_hash_finish_req(hdev->req, err);
+ stm32_hash_finish_req(hdev->req, 0);
return IRQ_HANDLED;
}
--
2.9.0
gcc warns that the length for the extra unaligned data in the hash
function may be used unaligned. In theory this could happen if
we pass a zero-length sg_list, or if sg_is_last() was never true:
In file included from drivers/crypto/stm32/stm32-hash.c:23:
drivers/crypto/stm32/stm32-hash.c: In function 'stm32_hash_one_request':
include/uapi/linux/kernel.h:12:49: error: 'ncp' may be used uninitialized in this function [-Werror=maybe-uninitialized]
#define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
Neither of these can happen in practice, so the warning is harmless.
However while trying to suppress the warning, I noticed multiple
problems with that code:
- On big-endian kernels, we byte-swap the data like we do for
register accesses, however this is a data stream and almost
certainly needs to use a single writesl() instead of series
of writel() to give the correct hash.
- If the length is not a multiple of four bytes, we skip the
last word entirely, since we write the truncated length
using stm32_hash_set_nblw().
- If we change the code to round the length up rather than
down, the last bytes contain stale data, so it needs some
form of padding.
This tries to address all four problems, by correctly
initializing the length to zero, using endian-safe copy
functions, adding zero-padding and passing the padded length.
I have done no testing on this patch, so please review
carefully and if possible test with an unaligned length
and big-endian kernel builds.
Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/crypto/stm32/stm32-hash.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 3c23a23e9ee5..b2ed2bccdb66 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -553,9 +553,9 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
{
struct stm32_hash_request_ctx *rctx = ahash_request_ctx(hdev->req);
struct scatterlist sg[1], *tsg;
- int err = 0, len = 0, reg, ncp;
+ int err = 0, len = 0, reg, ncp = 0;
unsigned int i;
- const u32 *buffer = (const u32 *)rctx->buffer;
+ u32 *buffer = (void *)rctx->buffer;
rctx->sg = hdev->req->src;
rctx->total = hdev->req->nbytes;
@@ -620,10 +620,13 @@ static int stm32_hash_dma_send(struct stm32_hash_dev *hdev)
reg |= HASH_CR_DMAA;
stm32_hash_write(hdev, HASH_CR, reg);
- for (i = 0; i < DIV_ROUND_UP(ncp, sizeof(u32)); i++)
- stm32_hash_write(hdev, HASH_DIN, buffer[i]);
-
- stm32_hash_set_nblw(hdev, ncp);
+ if (ncp) {
+ memset(buffer + ncp, 0,
+ DIV_ROUND_UP(ncp, sizeof(u32)) - ncp);
+ writesl(hdev->io_base + HASH_DIN, buffer,
+ DIV_ROUND_UP(ncp, sizeof(u32)));
+ }
+ stm32_hash_set_nblw(hdev, DIV_ROUND_UP(ncp, sizeof(u32)));
reg = stm32_hash_read(hdev, HASH_STR);
reg |= HASH_STR_DCAL;
stm32_hash_write(hdev, HASH_STR, reg);
--
2.9.0
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Arnd,
I've already push this fix for review last month, waiting the ack.
"
From: Lionel Debieve <[email protected]>
To: Herbert Xu <[email protected]>, "David S . Miller"
<[email protected]>, Maxime Coquelin <[email protected]>, Alexandre
Torgue <[email protected]>, <[email protected]>,
<[email protected]>, <[email protected]>
CC: Benjamin Gaignard <[email protected]>, Fabien Dessenne
<[email protected]>, Ludovic Barre <[email protected]>
Subject: [PATCH 1/1] crypto: stm32/hash - Remove uninitialized symbol
Date: Fri, 18 Aug 2017 15:54:01 +0200
"
Sorry if you receive this mail twice, I didn't see any mail in the mailing list, maybe server issue.
I'm reviewing your second part patch.
BR,
Lionel
> On 09/12/2017 11:35 AM, Arnd Bergmann wrote:
>> The error handling in stm32_hash_irq_thread passes
>> uninitialized data into stm32_hash_finish_req, as gcc
>> points out:
>> drivers/crypto/stm32/stm32-hash.c: In function 'stm32_hash_irq_thread':
>> drivers/crypto/stm32/stm32-hash.c:1088:2: error: 'err' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>> I could not tell what data should be passed there instead,
>> so this changes the code to always pass zero, making it
>> well-defined, though possibly still wrong. Please check.
>> Signed-off-by: Arnd Bergmann <[email protected]>
>> ---
>> drivers/crypto/stm32/stm32-hash.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>> diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
>> index b585ce54a802..3c23a23e9ee5 100644
>> --- a/drivers/crypto/stm32/stm32-hash.c
>> +++ b/drivers/crypto/stm32/stm32-hash.c
>> @@ -1067,7 +1067,6 @@ static int stm32_hash_cra_sha256_init(struct crypto_tfm *tfm)
>> static irqreturn_t stm32_hash_irq_thread(int irq, void *dev_id)
>> {
>> struct stm32_hash_dev *hdev = dev_id;
>> - int err;
>>
>> if (HASH_FLAGS_CPU & hdev->flags) {
>> if (HASH_FLAGS_OUTPUT_READY & hdev->flags) {
>> @@ -1085,7 +1084,7 @@ static irqreturn_t stm32_hash_irq_thread(int irq, void *dev_id)
>>
>> finish:
>> /*Finish current request */
>> - stm32_hash_finish_req(hdev->req, err);
>> + stm32_hash_finish_req(hdev->req, 0);
>>
>> return IRQ_HANDLED;
>> }
>
On Tue, Sep 12, 2017 at 11:35:38AM +0200, Arnd Bergmann wrote:
> The error handling in stm32_hash_irq_thread passes
> uninitialized data into stm32_hash_finish_req, as gcc
> points out:
>
> drivers/crypto/stm32/stm32-hash.c: In function 'stm32_hash_irq_thread':
> drivers/crypto/stm32/stm32-hash.c:1088:2: error: 'err' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> I could not tell what data should be passed there instead,
> so this changes the code to always pass zero, making it
> well-defined, though possibly still wrong. Please check.
>
> Signed-off-by: Arnd Bergmann <[email protected]>
This is already fixed in cryptodev.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Tue, Sep 12, 2017 at 11:35:39AM +0200, Arnd Bergmann wrote:
> gcc warns that the length for the extra unaligned data in the hash
> function may be used unaligned. In theory this could happen if
> we pass a zero-length sg_list, or if sg_is_last() was never true:
>
> In file included from drivers/crypto/stm32/stm32-hash.c:23:
> drivers/crypto/stm32/stm32-hash.c: In function 'stm32_hash_one_request':
> include/uapi/linux/kernel.h:12:49: error: 'ncp' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
>
> Neither of these can happen in practice, so the warning is harmless.
>
> However while trying to suppress the warning, I noticed multiple
> problems with that code:
>
> - On big-endian kernels, we byte-swap the data like we do for
> register accesses, however this is a data stream and almost
> certainly needs to use a single writesl() instead of series
> of writel() to give the correct hash.
>
> - If the length is not a multiple of four bytes, we skip the
> last word entirely, since we write the truncated length
> using stm32_hash_set_nblw().
>
> - If we change the code to round the length up rather than
> down, the last bytes contain stale data, so it needs some
> form of padding.
>
> This tries to address all four problems, by correctly
> initializing the length to zero, using endian-safe copy
> functions, adding zero-padding and passing the padded length.
>
> I have done no testing on this patch, so please review
> carefully and if possible test with an unaligned length
> and big-endian kernel builds.
>
> Fixes: 8a1012d3f2ab ("crypto: stm32 - Support for STM32 HASH module")
> Signed-off-by: Arnd Bergmann <[email protected]>
Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt