Re: [bug report] crypto: sun8i-ss - rework handling of IV

[LABBE Corentin <clabbe@baylibre.com>]

Le Thu, Aug 25, 2022 at 07:08:56PM +0300, Dan Carpenter a écrit :
> Hello Corentin Labbe,
> 
> The patch 359e893e8af4: "crypto: sun8i-ss - rework handling of IV"
> from May 2, 2022, leads to the following Smatch static checker
> warning:
> 
>     drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c:146 sun8i_ss_setup_ivs()
>     warn: 'a' is not a DMA mapping error
> 
>     drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c:213 sun8i_ss_cipher()
>     warn: 'rctx->p_key' is not a DMA mapping error

Hello

I dont know why I used an u32 and not a dma_addr_t.
I will work on a patch fixing this soon.

Thanks for the report.

> 
> drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
>     115 static int sun8i_ss_setup_ivs(struct skcipher_request *areq)
>     116 {
>     117         struct crypto_skcipher *tfm = crypto_skcipher_reqtfm(areq);
>     118         struct sun8i_cipher_tfm_ctx *op = crypto_skcipher_ctx(tfm);
>     119         struct sun8i_ss_dev *ss = op->ss;
>     120         struct sun8i_cipher_req_ctx *rctx = skcipher_request_ctx(areq);
>     121         struct scatterlist *sg = areq->src;
>     122         unsigned int todo, offset;
>     123         unsigned int len = areq->cryptlen;
>     124         unsigned int ivsize = crypto_skcipher_ivsize(tfm);
>     125         struct sun8i_ss_flow *sf = &ss->flows[rctx->flow];
>     126         int i = 0;
>     127         u32 a;
> 
> This needs to be a dma_addr_t a;
> 
>     128         int err;
>     129 
>     130         rctx->ivlen = ivsize;
>     131         if (rctx->op_dir & SS_DECRYPTION) {
>     132                 offset = areq->cryptlen - ivsize;
>     133                 scatterwalk_map_and_copy(sf->biv, areq->src, offset,
>     134                                          ivsize, 0);
>     135         }
>     136 
>     137         /* we need to copy all IVs from source in case DMA is bi-directionnal */
>     138         while (sg && len) {
>     139                 if (sg_dma_len(sg) == 0) {
>     140                         sg = sg_next(sg);
>     141                         continue;
>     142                 }
>     143                 if (i == 0)
>     144                         memcpy(sf->iv[0], areq->iv, ivsize);
>     145                 a = dma_map_single(ss->dev, sf->iv[i], ivsize, DMA_TO_DEVICE);
> --> 146                 if (dma_mapping_error(ss->dev, a)) {
> 
> This can't be true because of the 32/63 bit bug.
> 
>     147                         memzero_explicit(sf->iv[i], ivsize);
>     148                         dev_err(ss->dev, "Cannot DMA MAP IV\n");
>     149                         err = -EFAULT;
>     150                         goto dma_iv_error;
>     151                 }
>     152                 rctx->p_iv[i] = a;
> 
> But then only 32 bits are used later in the driver in ->p_iv[].  So it's
> more complicated than I thought.
> 
>     153                 /* we need to setup all others IVs only in the decrypt way */
>     154                 if (rctx->op_dir & SS_ENCRYPTION)
>     155                         return 0;
>     156                 todo = min(len, sg_dma_len(sg));
>     157                 len -= todo;
>     158                 i++;
>     159                 if (i < MAX_SG) {
>     160                         offset = sg->length - ivsize;
>     161                         scatterwalk_map_and_copy(sf->iv[i], sg, offset, ivsize, 0);
>     162                 }
>     163                 rctx->niv = i;
>     164                 sg = sg_next(sg);
>     165         }
>     166 
>     167         return 0;
>     168 dma_iv_error:
>     169         i--;
>     170         while (i >= 0) {
>     171                 dma_unmap_single(ss->dev, rctx->p_iv[i], ivsize, DMA_TO_DEVICE);
>     172                 memzero_explicit(sf->iv[i], ivsize);
>     173                 i--;
>     174         }
>     175         return err;
>     176 }
> 
> regards,
> dan carpenter